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

feat: auto-import for sway #5174

Merged
merged 36 commits into from
Oct 20, 2023
Merged

feat: auto-import for sway #5174

merged 36 commits into from
Oct 20, 2023

Conversation

sdankel
Copy link
Member

@sdankel sdankel commented Oct 5, 2023

Description

Closes #4782

Notes

This implementation relies on the diagnostics that are provided in the context of a code action request. Conveniently, only the diagnostics related to the symbol in the request are provided in the context. Since we also need the name of the symbol to look up, I added a DiagnosticData type to the metadata that is published with diagnostics. For the errors that indicate a missing symbol, this data is attached, and then used to look for possible imports when the code action request handler runs.

  • Added ProgramTypeKeyword symbol kind to distinguish them from other keywords
  • Added submodules_recursive to lexed and parsed modules because LSP wasn't collecting tokens inside of nested submodules. This implementation comes from typed module, I just made it generic via the HasSubmodules and HasModule traits.
  • Added TyIncludeStatement to the compiler, and span and mod_name to IncludeStatement to populate it. This was needed to identify where the end of the last mod statement is in the file.
  • Added span to UseStatment and TyUseStatement so that we can replace an entire Group statement with the new one. e.g. use a::b::{C, D, E};
  • Added CallPath to several of the typed tokens that were missing it.

Testing

  • integration tests for each type of item that can be imported
  • unit tests for the location of the generated import in the file

Limitations

I found several bugs / edge cases where auto-import doesn't work:

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@sdankel sdankel changed the title Sophie/import code action feat: auto-import for sway Oct 13, 2023
@sdankel sdankel marked this pull request as ready for review October 13, 2023 05:43
@sdankel sdankel requested review from a team October 13, 2023 05:43
@sdankel
Copy link
Member Author

sdankel commented Oct 17, 2023

Just tested this locally with the test example. Seems each import into the deep_mod use statement added a new line leading to a large gap between it and the next use statement. Are we able to avoid this?

Screenshot 2023-10-17 at 10 59 11 am

Good catch! Fixed.

JoshuaBatty
JoshuaBatty previously approved these changes Oct 17, 2023
Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

🚀

@JoshuaBatty JoshuaBatty requested a review from a team October 17, 2023 23:03
@sdankel sdankel requested a review from cr-fuel October 18, 2023 01:18
@sdankel sdankel enabled auto-merge (squash) October 18, 2023 01:18
@JoshuaBatty JoshuaBatty requested review from a team October 19, 2023 19:57
@sdankel sdankel requested a review from cr-fuel October 20, 2023 00:48
Copy link
Contributor

@IGI-111 IGI-111 left a comment

Choose a reason for hiding this comment

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

LGTM but I do insist on that name change as people seem to be easily confused about the fact that CallPath values from the typed AST are not guaranteed to be valid top level fullpaths, which this explicitly needs to be as I understand the usage.

@sdankel
Copy link
Member Author

sdankel commented Oct 20, 2023

LGTM but I do insist on that name change as people seem to be easily confused about the fact that CallPath values from the typed AST are not guaranteed to be valid top level fullpaths, which this explicitly needs to be as I understand the usage.

It doesn't need to be a full path actually, since I end up calling to_fullpath on it anyway. It's just there for symmetry with the other types (TyEnumDecl, TyStructDecl) to make the code more consistent.

@sdankel sdankel disabled auto-merge October 20, 2023 20:14
@sdankel sdankel requested a review from IGI-111 October 20, 2023 20:14
@IGI-111
Copy link
Contributor

IGI-111 commented Oct 20, 2023

Ah never mind that then

@IGI-111 IGI-111 enabled auto-merge (squash) October 20, 2023 20:16
@IGI-111 IGI-111 merged commit 4a09a4b into master Oct 20, 2023
31 checks passed
@IGI-111 IGI-111 deleted the sophie/importCodeAction branch October 20, 2023 21:18
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.

Insert missing imports for unresolved items.
4 participants