sway icon indicating copy to clipboard operation
sway copied to clipboard

LSP: Fix “Go to Definition” for struct generic parameters & add token-scan guard

Open Dannynsikak opened this issue 7 months ago • 6 comments

Description

closes #7025.

This pull request enhances the logic for resolving "Go to Definition" in the Session implementation of the Sway language server. The primary change introduces additional checks to handle type parameters within struct declarations more accurately, ensuring the resolution is restricted to the relevant struct context.

Improvements to "Go to Definition" logic:

  • Enhanced handling of type parameters: Added logic to identify and restrict type parameter lookups to the enclosing struct declaration. This ensures that type parameters are resolved only within the context of the struct they belong to.
  • Prevented performance issues: Introduced a limit of 1000 tokens when iterating over tokens in a file to avoid potential stack overflow or performance degradation in pathological cases.
  • Fallback to original behavior: Retained the original "Go to Definition" logic as a fallback for cases where the new type parameter handling does not apply.

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.

Dannynsikak avatar May 23 '25 10:05 Dannynsikak

please update title accordingly.

zees-dev avatar May 24 '25 01:05 zees-dev

please update title accordingly.

please ellaborate more , what title ?

Dannynsikak avatar May 24 '25 09:05 Dannynsikak

please update title accordingly.

please ellaborate more , what title ?

Thanks for the PR. I'll take a look at this on Monday. The PR tilte is simply "Dev". Can you rename this to better describe the PR. Thanks.

JoshuaBatty avatar May 24 '25 12:05 JoshuaBatty

please update title accordingly.

please ellaborate more , what title ?

Thanks for the PR. I'll take a look at this on Monday. The PR tilte is simply "Dev". Can you rename this to better describe the PR. Thanks.

dn

Dannynsikak avatar May 25 '25 08:05 Dannynsikak

please update title accordingly.

please ellaborate more , what title ?

Thanks for the PR. I'll take a look at this on Monday. The PR tilte is simply "Dev". Can you rename this to better describe the PR. Thanks.

dn

The title is still very generic; when one looks at merged PRs (or release notes), it's difficult to know at glance what the PR did - since you used the issue number as the title.. I have updated it accordingly to LSP: Fix “Go to Definition” for struct generic parameters & add token-scan guard.

zees-dev avatar May 25 '25 23:05 zees-dev

This PR needs test cases that reproduce the original bug from issue #7025 and verify the fix works correctly. Additionally, the current approach introduces O(n²) complexity and arbitrary iteration limits that need to be addressed before merging.

summary of fix

  • Add targeted tests to reproduce and verify fixes for stack overflows and infinite recursion in deep/cyclic ASTs.
  • Document the recursion guard approach and its usage in the codebase.
  • Ensure no O(n²) complexity or arbitrary iteration limits are introduced by the guard.
  • Clarify safety and efficiency of the recursion guard for LSP and compiler AST traversal.

Dannynsikak avatar Jun 02 '25 16:06 Dannynsikak

Thank you for your contribution, but this PR introduces significant architectural problems including arbitrary recursion limits, thread-local state pollution, removal of most tests, and complex nested loops in the token resolution logic that would require a complete rewrite to address safely.

We'd prefer to handle the struct generic parameter issue with a more focused approach that maintains our code quality standards and doesn't remove existing functionality.

JoshuaBatty avatar Aug 12 '25 06:08 JoshuaBatty