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 convert to call #4237

Merged
merged 6 commits into from
Feb 12, 2025
Merged

Conversation

giacomocavalieri
Copy link
Member

I found a couple of bugs in the "convert to call" code action testing it out on some real code:

  • It wouldn't work with module selects, moving only the bit after the .
  • It wouldn't work with module selects that cannot be inferred, moving only the bit after .
  • It would remove labels when piping into a capture with explicit labels written down

This PR fixes all of those issues

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you! Couple notes inline

/// // ^^^^^^ module_location
/// ```
///
module_location: SrcSpan,
Copy link
Member

Choose a reason for hiding this comment

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

The second int is redundant, it is the same as the first int in location.

Perhaps we want to have location be the whole thing and then have a field_start or something which is the location of the first character after the .? Would be nice to have the warning and errors from from after the dot rather than on it too so you don't have to press right after jumping to the error to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would love for the location to cover the entire field, I've always found it a bit odd that the module selection location doesn't cover the whole thing, and it means we always have to special case those in code actions that need to work on their entire span. I'll try and fix it!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Jak!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! fafcfb8

compiler-core/src/ast/tests.rs Outdated Show resolved Hide resolved
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!!

I incorrectly used the location of the whole "label + discard hole" as
the hole location, this meant that when removing a pipe and inserting
the value on the left of the `|>` in the hole I'd overwrite the label as
well
when a field access can't be inferred the invalid expression now cover
the entire thing instead of just the label part, that was previously
excluded
only the part after the `.` would be moved into the function call. Now
we keep track of the entire srcspan of the module select so we can move
the entire thing including the module before `.`
@lpil lpil force-pushed the fix-convert-to-call branch from cbde505 to 8a6ca7d Compare February 12, 2025 17:46
@lpil lpil enabled auto-merge (rebase) February 12, 2025 17:52
@lpil lpil merged commit aab05d4 into gleam-lang:main Feb 12, 2025
12 checks passed
@giacomocavalieri giacomocavalieri deleted the fix-convert-to-call branch February 12, 2025 18:22
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.

2 participants