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

Remove support for ExplicitResultTypes.skipLocalImplicits option #1209

Merged
merged 2 commits into from
Jul 11, 2020

Conversation

bjaglin
Copy link
Collaborator

@bjaglin bjaglin commented Jul 10, 2020

Fix #1198

Reverts #1181

Rationale: #1198 (comment)

This reverts commit 307c109, reversing
changes made to f7adc0e.
ExplicitResultTypesConfig uses the Scalameta parser to only compute
SemanticDB global symbols and then load those symbols with the
presentation compiler. It's not possible to use the same approach for
local symbols, so we should drop support for it.
@bjaglin
Copy link
Collaborator Author

bjaglin commented Jul 10, 2020

Unfortunately @giabao we'll have to revert your PR as this implementation of skipLocalImplicits (or any other given what's exposed at the moment) is not safe.

g.inverseSemanticdbSymbols(sym.value)
.find(s => g.semanticdbSymbol(s) == sym.value)
.getOrElse(g.NoSymbol)
GlobalProxy.typedTreeAt(g, gpos)
Copy link
Collaborator Author

@bjaglin bjaglin Jul 10, 2020

Choose a reason for hiding this comment

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

@olafurpg is this called just for its side effect ad8a4bb? That goes far beyond my understanding, but when it was added in 162db39#diff-0771fc97e5f15a43af8af2a9e65cbc62R137, there was no explicit side effect so I wonder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. This method performs “positioned type checking”, which loads up all the necessary symbols from the classpath. The return value is helpful in other situations like in metals where we use this method to implement parameter hints or completions

@bjaglin bjaglin marked this pull request as ready for review July 10, 2020 20:35
Copy link
Collaborator

@mlachkar mlachkar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@olafurpg olafurpg 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!

@bjaglin bjaglin merged commit e9a16a8 into scalacenter:master Jul 11, 2020
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.

fix ci-213-windows
4 participants