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.

Sign up for GitHub

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

Jump to bottom

Fix widen types before checking an implicit view exists #18719

Merged
merged 3 commits into from
Oct 19, 2023
Merged

Fix widen types before checking an implicit view exists #18719

merged 3 commits into from
Oct 19, 2023

Conversation

dwijnand
Copy link
Member

Fixes #18650

@dwijnand
report: Trim down enriched error messages
b120977 
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.
@dwijnand
report: Add a Y-flag to disable enriched error messages
e6fe320
@dwijnand
Fix widen types before checking an implicit view exists
8a2773f 
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
dwijnand
dwijnand commented Oct 19, 2023
View reviewed changes
compiler/src/dotty/tools/dotc/typer/Implicits.scala
@@ -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
odersky
odersky approved these changes Oct 19, 2023
View reviewed changes
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
This was referenced Oct 20, 2023
Merged
Closed
Closed
odersky added a commit to dotty-staging/dotty that referenced this pull request Oct 25, 2023
@odersky
Avoid crash arising from trying to find conversions from polymorphic …
f995a8c 
…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.
@odersky odersky mentioned this pull request Oct 25, 2023
Merged
@dwijnand dwijnand mentioned this pull request Nov 20, 2023
Open
@Kordyjan Kordyjan mentioned this pull request Nov 23, 2023
Merged
@dwijnand dwijnand mentioned this pull request Nov 25, 2023
Merged
@dwijnand
Copy link
Member Author

Tweaked in #18727.

@dwijnand dwijnand mentioned this pull request Nov 25, 2023
Closed
Kordyjan added a commit that referenced this pull request Dec 8, 2023
@Kordyjan
Backport "Fix widen types before checking an implicit view exists" to…
435daa9 
… 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
@dwijnand dwijnand mentioned this pull request Dec 15, 2023
Merged
odersky added a commit that referenced this pull request Dec 17, 2023
@odersky
Avoid diagnostic message forcing crashing the compiler (#19113)
10f2c10 
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.
@som-snytt som-snytt mentioned this pull request Dec 23, 2023
Closed
@som-snytt som-snytt mentioned this pull request Dec 23, 2023
Open
dwijnand pushed a commit to dwijnand/scala3 that referenced this pull request Feb 21, 2024
@odersky @dwijnand
Avoid diagnostic message forcing crashing the compiler (scala#19113)
2459549 
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
@odersky @dwijnand
Avoid diagnostic message forcing crashing the compiler (scala#19113)
e300188 
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)
WojciechMazur pushed a commit that referenced this pull request Jun 22, 2024
@odersky @WojciechMazur
Avoid crash arising from trying to find conversions from polymorphic …
46fc93d 
…singleton types

This is an alternative fix for #18695, which already got fixed in a different way by #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 #18719.

[Cherry-picked f995a8c]
WojciechMazur pushed a commit that referenced this pull request Jun 23, 2024
@odersky @WojciechMazur
Avoid crash arising from trying to find conversions from polymorphic …
e90d376 
…singleton types

This is an alternative fix for #18695, which already got fixed in a different way by #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 #18719.

[Cherry-picked f995a8c]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

Assignees

@dwijnand dwijnand

Labels
None yet
Projects
None yet
Milestone
3.3.2
Development

Successfully merging this pull request may close these issues.

regression: crash when summoning type equivalence
3 participants
@dwijnand @odersky @Kordyjan