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

bugfix: Print local type aliases properly #4188

Merged
merged 1 commit into from Jul 25, 2022
Merged

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Jul 25, 2022

Previously, we would not print local type names properly and instead just print "_". Now we search for the display name of that local type alias and display it instead.

Fixes #4186

@tgodzik tgodzik requested review from dos65 and kpodsiad July 25, 2022 09:44
Comment on lines 562 to 564
// local type aliases
case ValueSignature(tp: s.TypeRef) if tp.symbol.isLocal =>
textDocument.symbols.collectFirst {
Copy link
Member

Choose a reason for hiding this comment

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

what if type alias is defined in other file? It isn't local then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it should work normally, the problem is with printing local type aliases as far as I was able to check. Or do you have a counter example?

Copy link
Contributor Author

@tgodzik tgodzik Jul 25, 2022

Choose a reason for hiding this comment

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

Changed the test to include a nonlocal type.

Copy link
Member

Choose a reason for hiding this comment

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

Right, non local type was working, my bad.

Comment on lines 562 to 564
// local type aliases
case ValueSignature(tp: s.TypeRef) if tp.symbol.isLocal =>
textDocument.symbols.collectFirst {
Copy link
Member

Choose a reason for hiding this comment

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

Right, non local type was working, my bad.

Copy link
Member

@dos65 dos65 left a comment

Choose a reason for hiding this comment

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

@tgodzik Looks good but I'm wondering why we have this line in printer ?

I removed it locally and without it the new test works well without changes in SyntheticDecorationProvider

Previously, we would not print local type names properly and instead just print "_". Now we search for the display name of that local type alias and display it instead.

Fixes scalameta#4186
@tgodzik
Copy link
Contributor Author

tgodzik commented Jul 25, 2022

@tgodzik Looks good but I'm wondering why we have this line in printer ?

I removed it locally and without it the new test works well without changes in SyntheticDecorationProvider

Good catch! I changed that a bit to have _ as the last resort.

Copy link
Member

@dos65 dos65 left a comment

Choose a reason for hiding this comment

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

👍

@tgodzik tgodzik merged commit 81b014d into scalameta:main Jul 25, 2022
@tgodzik tgodzik deleted the type-wrong branch July 25, 2022 16:05
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.

Incorrect inferred type for type alias in for comprehension
3 participants