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

Fix widen types before checking an implicit view exists #18719

Merged
merged 3 commits into from
Oct 19, 2023

Conversation

dwijnand
Copy link
Member

Fixes #18650

The implementation here comes from porting from nsc.  But I think
things were done differently enough with its Global state, that make it
not translate nicely here.  So I'm paring it down to what I know works
(at least for me): versions, settings, compilation unit.  Not the
tree/symbol/site stuff.
It's not possible to convert a method, such as a polymorphic method,
such as `makeChurch`, into another type - we need to widen out the
TermRef to discover the underlying PolyType which isn't a value type.
Failing to do so will create a dummyTreeOfType, which will then be
attempted to be applied, which eventually causes an assertion crash
while building the tpd.TypeApply.
@dwijnand dwijnand requested a review from odersky October 18, 2023 23:39
@dwijnand dwijnand assigned odersky and unassigned dwijnand Oct 18, 2023
@dwijnand dwijnand marked this pull request as ready for review October 18, 2023 23:39
@@ -850,7 +850,7 @@ trait Implicits:
&& !to.isError
&& !ctx.isAfterTyper
&& ctx.mode.is(Mode.ImplicitsEnabled)
&& from.isValueType
&& from.widen.isValueType
Copy link
Member Author

Choose a reason for hiding this comment

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

Counter-example:

  given foo[A]: (Ctx => Int) = _ => 42

  val works = get[Int](using summon[Ctx => Int])
  val fails = get[Int]

using the example from #16453 - with this change (or !imp.underlying.isInstanceOf[PolyType] in ignoredConvertibleImplicits) the suggestion of foo will be dropped, even though it's a correct suggestion.

@dwijnand dwijnand removed the request for review from odersky October 19, 2023 09:54
@dwijnand dwijnand assigned dwijnand and unassigned odersky Oct 19, 2023
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

LGTM, both the improvements to enriched error messages and the fix in Implicits.

@odersky odersky merged commit bf994f9 into scala:main Oct 19, 2023
18 checks passed
@dwijnand dwijnand deleted the SubType_refl-wrecking-havoc branch October 19, 2023 11:34
odersky added a commit to dotty-staging/dotty that referenced this pull request Oct 25, 2023
…singleton types

This is an alternative fix for scala#18695, which already got fixed in a different way by scala#18719.
This PR adds the actual tests, and leaves in the fix as a defensive measure in case the situation
arises by some other means than the one foxed in scala#18719.
@dwijnand
Copy link
Member Author

Tweaked in #18727.

Kordyjan added a commit that referenced this pull request Dec 8, 2023
… LTS (#19071)

Backports #18719 to the LTS branch.

PR submitted by the release tooling.
@Kordyjan Kordyjan added this to the 3.3.2 milestone Dec 14, 2023
odersky added a commit that referenced this pull request Dec 17, 2023
Using issue #18650 as the reference (but issue #18999 is another option)
building on the fix in PR #18719 (refined in PR #18727) as well as the
fix in PR #18760, I'm trying to make a more root change here by making
sure that message forcing only occurs with `hasErrors`/`errorsReported`
is true, so as to avoid assertion errors crashing the compiler.
dwijnand pushed a commit to dwijnand/scala3 that referenced this pull request Feb 21, 2024
Using issue scala#18650 as the reference (but issue scala#18999 is another option)
building on the fix in PR scala#18719 (refined in PR scala#18727) as well as the
fix in PR scala#18760, I'm trying to make a more root change here by making
sure that message forcing only occurs with `hasErrors`/`errorsReported`
is true, so as to avoid assertion errors crashing the compiler.

(cherry picked from commit 10f2c10)
dwijnand pushed a commit to dwijnand/scala3 that referenced this pull request Mar 5, 2024
Using issue scala#18650 as the reference (but issue scala#18999 is another option)
building on the fix in PR scala#18719 (refined in PR scala#18727) as well as the
fix in PR scala#18760, I'm trying to make a more root change here by making
sure that message forcing only occurs with `hasErrors`/`errorsReported`
is true, so as to avoid assertion errors crashing the compiler.

(cherry picked from commit 10f2c10)
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.

regression: crash when summoning type equivalence
3 participants