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

LSP: fix hovering over a string #26177

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

DanilaFe
Copy link
Contributor

@DanilaFe DanilaFe commented Oct 30, 2024

Updates to the Dyno resolver have exposed some bugs that once again break hovering over string with the resolver enabled. This PR fixes a number of minor issues that cause crashes. The changes are as follows:

  1. Handle type query domains. Thus, [?D] now resolves to a generic array type as opposed to 'Unknown'.
  2. Treat passing to unknown type as instantiation. This is important for cases in which type queries need to be populated. They are only populated from substitutions, but substitutions are not created for completely unknown formals, because prior to this change, a completely unknown formal can be passed to without instantiating (i.e., without substitution).
  3. Reject candidates that fail due to inability to infer type queries. After computing a formal type, we traverse the formal AST to find out what the type queries are. This may fail (e.g., if the formal AST doesn't match up with the instantiated type). Thus, the post-query type will be unknown. This is an error, since it indicates that the type queries couldn't be properly made sense of and incorporated into the final type info. In such cases, reject the candidate.
    • A concrete example: formal ?k*?t, actual int. The initial type is unknown; after instantiation, the formal type is int. When resolving queries, we can't figure out how to spit int across ?k*?t, leaving type queries unknown. When re-combining the formal type with query info, ?k*?t is again unknown since the queries were not known. This means we couldn't instantiate ?k*?t with int, and the candidate is rejected.
  4. Handle unknown actuals (needed for out-formals) in builtin functions. We have some builtin functions like type/param casting to string, handled by the compiler. They did not expect null actual types, but this can happen sometimes (e.g., when the call site allows for the possibility of out-formals being matched with the actual). This change adjusts the code to handle that by simply not attempting to resolve builtins in that case (none of the have out formals, so this is sound).
  5. Skip unknown arguments when building ranges. Resolving ranges boils down to resolving a function call. Dyno skips resolving normal function calls under certain conditions (e.g., if the argument types could not be inferred). However, it doesn't do so for ranges, which made it possible for 'Unknown' or otherwise invalid actuals to slip into the call resolution process. This commit adjusts the resolver to re-use the skipping logic.

Reviewed by @dlongnecke-cray -- thanks!

Testing

  • dyno tests
  • CLS tests

This is important for cases in which type queries need to be populated.
They are only populated from substitutions, but substitutions
are not created for completely unknown formals, because
prior to this commit, a completely unknown formal can be
passed to without instantiating (i.e., without substitution).
This commit changes that.

Signed-off-by: Danila Fedorin <[email protected]>
After computing a formal type, we traverse the formal AST to find
out what the type queries are. This may fail (e.g., if the formal
AST doesn't match up with the instantiated type). Thus, the post-
query type will be unknown. This is an error, since it indicates
that the type queries couldn't be properly made sense of and
incorporated into the final type info. In such cases, reject the
candidate.

A concrete example: formal ?k*?t, actual int. The initial type
is unknown; after instantiation, the formal type is int. When
resolving queries, we can't figure out how to spit init across
?k*?t, leaving type queries unknown. When re-combining the formal
type with query info, ?k*?t is again unknown since the queries
were not known. This means we couldn't instantiate ?k*?t with int,
and the candidate is rejected.

Signed-off-by: Danila Fedorin <[email protected]>
We have some builtin functions like type/param casting to string,
handled by the compiler. They did not expect `null` actual types,
but this can happen sometimes (e.g., when the call site allows
for the possibility of `out`-formals being matched with the actual).
This commit adjusts the code to handle that.

Signed-off-by: Danila Fedorin <[email protected]>
Resolving ranges boils down to resolving a function call. Dyno
skips resolving normal function calls under certain conditions
(e.g., if the argument types could not be inferred). However,
it doesn't do so for ranges, which made it possible for 'Unknown'
or otherwise invalid actuals to slip into the call resolution
process. This commit adjusts the resolver to re-use the skipping
logic.

Signed-off-by: Danila Fedorin <[email protected]>
Copy link
Contributor

@dlongnecke-cray dlongnecke-cray left a comment

Choose a reason for hiding this comment

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

LGTM, really well documented.

@DanilaFe DanilaFe merged commit 7ba4bc7 into chapel-lang:main Oct 30, 2024
7 checks passed
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