Subject: Re: bug#61302: 29.0.60; rust-ts-mode does not show function-invocation on field-properties

From: Jostein Kjønigsen <[email-masked]>
Date: Mon, 13 Feb 2023 11:17:01 +0100

Hey everyone.

When I was filing the initial bug-report, I sort of noticed how 
rust-ts-mode was ... lets say not as consistent or refined about things 
as the other -ts-modes have turned out to be, but for the time being I 
decided to focus on functions-calls only, because that seemed to be the 
biggest omission. That said, I did notice other parts had room for 
improvement too.

No need for me to file a bug-report for that now though, as I see you 
two have done some very thorough work in the meantime :D

If I were to complain about only -1- thing in that previous effort, it 
would be that you've compared rust-ts-mode to GitHub, VSCode, IntelliJ 
and what not ... but not rust-mode from MELPA! I think lots of the 
rust-ts-mode users will be coming from that major-mode, rather than 
other editors, so IMO it's quite a handy reference for how people might 
expect things to be.

Below is a screenshot which puts rust-ts-mode (with the latest patches 
from this thread) head to head against rust-mode from MELPA:

 From what I can tell, neither of them is perfect yet, but they both get 
some things right:

  * rust-ts-mode: function invocations :)
  * rust-ts-mode handles constants better (also escape-sequences, but
    not seen in this sample)
  * rust-mode: consistently fontify annotations (notice they are missing
    in rust-ts-mode, line 12 and 14). Also rust-mode use
    font-lock-preprocessor-face, which I think as a more appropriate
    face for this kind of syntax, than font-lock-constant-face (used in
    rust-ts-mode).
  * rust-mode: is able to handle nested macro-invocations. See line 42
    and 44 above. From what I can tell, this seems to be due to a
    short-coming in the tree-sitter grammar for rust, and we may be able
    to fix it upstream, instead of monkey-patch things based on regexp's
    in rust-ts-mode

As for things which are less great in rust-ts-mode:

  * some code does not seem to get fontified at all (types, keywords,
    etc). Line 14-17.
  * it seems to fontify all variables using font-lock-variable-name-face
    all over, regardless of it is a declaration or not. I realize this
    is not 100% consistent throughout the Emacs-verse, but I know other
    -ts-modes have aimed for declaration only, and so does rust-mode
    from MELPA too (although with some consistency-issues) which this
    would be replacing.
  * it does not seem to handle ::* imports properly? See line 9. The way
    I understand it, things preceeding the ::* should be considered a
    namespace too?
  * I know imports are difficult to be 100% accurate about, as seen in
    this thread. Are we importing a module or a class? Impossible to
    tell without looking at the referenced code! Aiming for visual
    consistency may be a better goal than 100% correctness, if the AST
    we're getting don't provide good enough information? (This has been
    done in other modes too)

With utmost respect for all the work put in so far (rust is a 
complicated language after all), and I realize this is subjective, to me 
rust-ts-mode does not yet seem /quite there/, in terms of correctness or 
consistency. And as I'm sure Eli will remind us, Emacs 29 release is 
getting very, very, very close.

Could it be an option to not have rust-ts-mode as part of Emacs 29, but 
leave it in git master for now? That would leave us time to sort out all 
these things properly, and also have good time to decide the things 
which actually needs to be agreed upon, before implementing the final fixes?

Just my 2 cents.

--
Jostein

On 2/10/23 04:44, Randy Taylor wrote:

> On Thursday, February 9th, 2023 at 16:19, Dmitry Gutov<[email-masked]>  wrote:
>> On 09/02/2023 05:38, Randy Taylor wrote:
>>
>>>> What if it looked like this:
>>>>
>>>>     let date = DateTime::<chrono::utc>::from_utc(date, chrono::utc);
>>>>
>>>> If we decide purely based on capitalization, then I guess the rule
>>>> should be present in both lists (with capitalized? regexp in one, and
>>>> !capitalized? regexp in another), and a few more rules should be
>>>> duplicated as well.
>>> In both cases, utc is still a type even if it's not capitalized.
>>> My patch addresses this.
>> So the end of a scoping chain must also be either a type or a method
>> call? We might be able to use that, somehow.
> I believe so (with the exception of use declarations as you note).
> Not familiar enough with Rust to say for sure :).
>
>> Though the 'use' declarations might be exceptions, e.g.
>>
>>    use crate::foo::baz::foobaz;crate
>>
>> or
>>
>>    use std::{fmt, fs, usize};
>>
>> (fmt and fs are modules, not types).
>>
>>>> This becomes a little more painful semantically, given that the first
>>>> 'utc' in the example above is parsed into a (type_identifier) node, not
>>>> just (identifier).
>>>>
>>>>>> On a distantly related note, we have terms like 'usize' which is
>>>>>> normally a type (and highlighted as such), but can also feature in
>>>>>> expressions like
>>>>>>
>>>>>>      let row = usize::from_str_radix(row, 10).map_err(|_| error())?;
>>>>>>
>>>>>> where it is now highlighted with font-lock-constant-face. Should we try
>>>>>> to do anything about that? If there is a limited number of built-in
>>>>>> types in that situation (e.g. all of them primitives), we could handle
>>>>>> that with a regexp.
>>>>> Right. I think it makes sense to handle the primitives with a regex.
>>>>> I'm not sure if there's anything else beyond those.
>>>>> There's a list of them here:https://doc.rust-lang.org/reference/types.html
>>>>> I think it would only apply to the numerical and textual types.
>>>> So 'usize' in the above is definitely a "type", not a "module"?
>>> I think so. You can see on usize's documentation page (https://doc.rust-lang.org/std/primitive.usize.html)
>>> that it provides that function, amongst many others.
>> I was thinking that it might also be referring to (apparently
>> deprecated)https://doc.rust-lang.org/std/usize/index.html.
> That module only provides the constants listed on that page.
> The usize type itself provides a bunch of constants and functions (same for the rest of the primitives).
>
> I'm curious how other editors/IDEs highlight this stuff, but I don't have any on hand ATM.
>
>> Sorry, I'm not really familiar with Rust. E.g. in Ruby every class can
>> also serve as a "module" in the scoping sense. As a result we highlight
>> both classes and modules with font-lock-type-face. This could also be an
>> option here, if everything else fails.
>>
>> But we could also highlight based on a "role" (constant if it's used as
>> a scope, and type if it's used as a type).
>>
>> Although one could say that 'Path' in
>>
>>    Some(Path::new("./foo"))
>>
>> is being used as a type as well, and 'Some' too. So it might not be the
>> best fit.
>>
>> Speaking of 'usize' again, what if some lib or the app defines an
>> 'usize' module for its custom functions acting on it? E.g.
>> 'my::app::usize'. A simple regexp matcher will probably highlight it as
>> a type as well.
> I don't think we should worry about those cases IMO.
>
>>>>>> Or vice versa, in
>>>>>>
>>>>>>      use std::{fmt, fs, usize};
>>>>>>
>>>>>> should 'fmt', 'fs' and 'usize' be highlighted with
>>>>>> font-lock-constant-face rather than font-lock-type-face?
>>>>> They should indeed be highlighted with font-lock-constant-face because they are modules.
>>>>> We assume the types will be capitalized since that's all we can really do (and it's the convention anyway).
>>>> If they're modules here, I suppose they should be highlighted the same in
>>>>
>>>>     let row = usize::from_str_radix(...)
>>>>
>>>> as well. The bright side is that will make a more complex regexp
>>>> (enumerating the lowercase named types) unnecessary.
>>> Yes, except for the primitives.
>>>
>>> I have attached a patch which I think addresses most of the concerns (although I've been at it for a few hours and my brain is mush now).
>>>
>>> The patch does the following:
>>> - Separates import-related stuff and module use by leveraging the use_declaration query (simplifying things greatly IMO).
>>> - Highlights primitive types used in scoped_identifiers.
>>> - Properly highlights types belonging to a module no matter how deep it is (or your money back guaranteed!).
>>> - Maybe some other stuff I forgot. I'm too tried now :).
>> Thank you, I can sympathize -- this stuff gets complicated.
>>
>> Some problems from my testing:
>>
>> 1. If I keep treesit-font-lock-level at its default value (3), some
>> stuff gets misfontified:
> Sorry, I have only been testing with level 4.
> This is also why I want type and module combined into one so we don't have to deal with this headache.
>
>>    use std::collections::hash_map::{self, HashMap};
>>
>> 'hash_map' is highlighted as a type. 'HashMap' is not highlighted at all.
>>
>>    use std::{fmt, fs, usize};
>>
>> Only 'use' is highlighted here.
> This is because of how things are broken out into the module feature.
> That some highlighting for those occurs is by overlap of queries in the type feature.
> Which again is why I think module should be part of type.
>
>>    test::test1();
>>
>> 'test1' is highlighted as a type (we discussed this problem with
>> highlighting types by default -- it becomes necessary to filter out
>> function calls, either with more complex queries, or with custom
>> highlighter functions).
> Right, I added a query to filter that out now.
>
>> 2. If I switch to treesit-font-lock-level 4:
>>
>>    let boxed_i32 = Box::new(5_i32);
>>
>> 'Box' is highlighted with font-lock-constant-face. I think it's a type,
>> though.
> Oops, I accidentally removed the rule for that. Added it back.
>
>> Also here's a pre-existing problem mentioned above:
>>
>>    use std::{fmt, fs, usize};
>>
>> 'fmt' and 'fs' are not types. But they are highlighted with
>> font-lock-type-face.
> This is really weird, I can reproduce it with emacs -Q but not with my normal config...
> Also with emacs -Q this:
> let date = DateTime::<chrono::hey::there::Utc>::from_utc(date, chrono::cool::this::Utc);
>
> highlights incorrectly, where "there" is font-lock-variable-name-face. But with my normal config everything is fine.
>
> I'll look into it tomorrow. Not really sure what in my config could cause this...
>
>>> A few questions:
>>> - Should module be moved to level 3 to be with type?
>>> - Do we still want the module feature, or should this stuff be put into type?
>> I suppose we should iron some kinds out first to get a better understanding.
> Attached a new patch hopefully addressing most of the problems you ran into (minus the level 3 use declaration highlights).
> Especially after the problems you ran into at level 3, I strongly think the module queries should get thrown into type (and they make sense there anyway IMO). Then all those issues go away.