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

Fix completion of enum and fault methods #111

Merged
merged 19 commits into from
Jan 27, 2025

Conversation

PgBiel
Copy link
Contributor

@PgBiel PgBiel commented Jan 26, 2025

Partial fix for Fixes #110, with fault as a drive-by. These changes were motivated by the method search work for #107

Not fully finished, but opening the PR in case of initial feedback.

Issues identified and fixed:

  • Missing method completion for enum instance variables (CoolEnum inst; followed by inst.methodNa).
  • Enum values wrongly suggested for enum instance variables (CoolEnum inst; followed by inst.SOME_ENUM_VALU shouldn't suggest anything).
  • Missing method detection and search for faults.
  • Missing method completion for fault instance variables (similar to above).
  • Fault constants wrongly suggested for fault instance variables (similar to above).
  • Fixed enum parsing: Changing the backing type is optional. We were expecting it to always be present, leading to associated values being ignored when the backing type wasn't explicitly specified. This was fixed in node_to_enum.go.
  • Missing associated value detection and search for enum instance variables.
  • Missing associated value completion for enum values and enum instance variables.
  • Missing method completion for explicit enum values (i.e. CoolEnum.SOME_ENUM_VALUE.methodNa should provide suggestions where appropriate).
  • Missing method completion for explicit fault constants (same).

Also, updated the stdlib shims to accommodate for an updated FaultConstantBuilder API, as well as EnumeratorBuilder API, to include their modules and parent enum/fault names.

Some notable observed changes to the stdlib shims:

  • Some module paths changed. No idea why.
  • Macro trailing arguments, such as @body, are now being properly detected and included in the arguments for macros.
  • Some previously missing types are now being included.

Extra

I've gone ahead and greatly simplified some of the existing tests by automatically fetching the cursor position. Take a look!

Implementation

  • For method completion and search, I simply used what structs do.
  • Regarding wrong completions on instance variables, search results now track whether the found result (e.g. a type) is precisely the symbol that was originally being searched (i.e. the cursor was over the type name) - then membersReadable is true (yep, can read enum values and fault constants from this result); or if it is strictly a parent type of the symbol under the cursor (i.e. it was originally a variable, even though the result returned its type) - then membersReadable is false (do not suggest enum values and fault constants here).
  • For associated value completion, I simply started checking associated values on each enum and fault instance if membersReadable is false (indicates the symbol is not an instance but the type itself).
  • For method completion on explicit constants of enums and faults, I simply stored the parent Enum / Fault name alongside the constant. This, combined with the constant's module, is enough to query for the parent type and access its methods on completion and search.

TODO:

All done!

Notes on AST

I can't really evaluate how much this could impact a rebase of the AST implementation at the moment as I haven't taken a deep look at the PR, but I think it's noteworthy that this PR contains some refactors regarding method search which will probably make it easier to reimplement.

@pherrymason
Copy link
Owner

pherrymason commented Jan 26, 2025

Take into account that code inside search will be completely scrapped.
I really feel like this is already fixed in next version and feel bad for you spending so many efforts on the current search code which is a mess to follow and difficult to tweak.

That being said, new tests you write to support this new features/fixes will be really useful anyway

@PgBiel
Copy link
Contributor Author

PgBiel commented Jan 26, 2025

Don't worry, I'm ok with having the search changes be scrapped. There are still a few benefits in moving this forward that I'm considering:

  1. Should you cut a release before the AST changes are finished (which seems fairly reasonable if they get too large to be finished and tested in time), these fixes will already be there, which is nice.

  2. We can better understand the problem and its solution, which should lead to a higher-quality implementation in the rewrite.

  3. As you mention, the tests will remain valid, so that will be helpful as well.

And, more importantly specifically for me, this small refactor, as well as by getting used to this part of the codebase, will make it easier for me to draft out an initial implementation of the distinct type, which can then be perfected with the AST search.

@PgBiel PgBiel marked this pull request as ready for review January 27, 2025 01:47
@PgBiel
Copy link
Contributor Author

PgBiel commented Jan 27, 2025

Good stuff! Overall, it wasn't pretty hard. I'd say writing the tests took much more time 😅, but at least that let me find some additional bugs along the way. 😄

@PgBiel PgBiel mentioned this pull request Jan 27, 2025
11 tasks
@pherrymason pherrymason merged commit dfaaeda into pherrymason:main Jan 27, 2025
2 checks passed
@pherrymason pherrymason mentioned this pull request Jan 28, 2025
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No completion for enum methods or associated values
2 participants