Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

highlights: rust: adjust enum variant recognition #8957

Merged

Conversation

rosefromthedead
Copy link
Contributor

Now, enum variants with no arguments are recognised:

match x {
    Here => {},
}
ErrorKind::Here.clone();
module::Enum::AndHere.into();

and have been tested to not be falsely recognised:

Here.clone();
module::OrHere.into(); // probably unit structs, not variants

but still aren't recognised in the absence of a field access, method call or match arm because I think we'd need to put a PascalCase::PascalCase rule for that everywhere where expressions can appear (to rule out unit structs) which is likely possible but definitely annoying.

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-language-support Area: Support for programming/text languages labels Nov 30, 2023
@kirawi
Copy link
Member

kirawi commented Nov 30, 2023

So, the reason why I settled on highlighting these as @constructor is that there is ambiguity between destructing structs and enums. For example,

match x {
    // Can either be a struct or an enum with named fields
    Foo { a, b, c } => {}
    // Can either be a tuple struct or an enum with fields
    Bar(a, b, c)
}

@rosefromthedead
Copy link
Contributor Author

You're right that there's ambiguity, and I see these ways out of it:

  1. Give up on @type.enum.variant. Anything with PascalCase in expression or pattern position would be a constructor, but the same idents in type position would remain as @type.
  2. Try to improve heuristics so that @type.enum.variant is correct as often as possible. IMO the current situation where the match pattern Some(x) is an enum variant but the match pattern None is not is surprising to look at, which is why this PR is here, but there is still more work to do to get this right.

I was originally aiming for (2), but (1) would be a lot more robust and more concise to implement, and I think I'd still be happy with a theme based on that :) so I'll give it a go.

@kirawi
Copy link
Member

kirawi commented Dec 2, 2023

I don't feel like this is quite the right away to approach this because now it creates further ambiguity between enums and structs outside of pattern matching. I think the most correct way to address this is to have locals.scm track enums in a file (and potentially use stackgraphs in the future) and use semantic highlighting from the LSP, the latter of which has an implementation in an old PR: #6102

I have been meaning to get to it, but life has been busy. But, it should fix your issues with the inconsistent highlighting once I or someone else continues that work. This PR, if merged, would likely need to be at least partially reverted in that case for proper token tracking.

@@ -40,7 +40,7 @@
; ---

(self) @variable.builtin
(enum_variant (identifier) @type.enum.variant)
(enum_variant (identifier) @constructor)
Copy link
Member

@kirawi kirawi Dec 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for this change, I am fine with the rest of the PR (since it is technically used in the form of constructors) as long as these changes are tracked to be possibly at least partially reverted once semantic highlighting is implemented. I don't know what the other maintainers think though.

@kirawi kirawi added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 6, 2023
@kirawi
Copy link
Member

kirawi commented Dec 19, 2023

Are you still interested in this PR? The comment before my last one may have given the idea I was rejecting the change all together, which is not the case.

Enum variants and (tuple) structs are indistinguishable in general, so we
mark any PascalCase pattern or expression as a "constructor", which
covers all three.
@rosefromthedead
Copy link
Contributor Author

Thanks for the poke, I had just forgotten :)

@kirawi kirawi added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 24, 2023
@pascalkuthe pascalkuthe merged commit a680b2e into helix-editor:master Jan 2, 2024
6 checks passed
woojiq pushed a commit to woojiq/helix that referenced this pull request Jan 13, 2024
Enum variants and (tuple) structs are indistinguishable in general, so we
mark any PascalCase pattern or expression as a "constructor", which
covers all three.
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
Enum variants and (tuple) structs are indistinguishable in general, so we
mark any PascalCase pattern or expression as a "constructor", which
covers all three.
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
Enum variants and (tuple) structs are indistinguishable in general, so we
mark any PascalCase pattern or expression as a "constructor", which
covers all three.
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
Enum variants and (tuple) structs are indistinguishable in general, so we
mark any PascalCase pattern or expression as a "constructor", which
covers all three.
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Enum variants and (tuple) structs are indistinguishable in general, so we
mark any PascalCase pattern or expression as a "constructor", which
covers all three.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants