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

Survive TypeErrors in isMatchedBy #15675

Merged
merged 1 commit into from
Jul 15, 2022
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jul 14, 2022

Fixes #15673

@odersky
Copy link
Contributor Author

odersky commented Jul 14, 2022

How I fixed it:

At first, I was a bit puzzled why a change to hasKnownMembers would have an effect like the one seen in #15673. The test case was really simple, so that was a plus. I compiled with -Xprompt and looked at the stack trace. That indicated that the error was a TypeError that was raised somewhere else and that was caught and issued in typedUnadapted. I grepped for the error message and found the method where it was issued: TypeRef#argDenot. I converted the TypeError to AssertionError to see the stack trace where it was issued: isMatchedBy in ProtoTypes. That explained why it would appear now as a regression: hasUnknownMembers overapproximates less in #14762, so the failing code path after the hasUnknownMembers in isMatchedBy is now taken where previously it was masked.

The question was still how to fix it. The problem was as explained in the new comment in hasmatchingMember

          // We have a type `CC[A]#C` where `CC`'s upper bound is `[X] => Any`, but
          // the current constraint stipulates CC <: SeqOps[...], where `SeqOps` defines
          // the `C` parameter. We try to resolve this using `argDenot` but `argDenot`
          // consults the base type of `CC`, which is not `SeqOps`, so it does not
          // find a corresponding argument. In fact, `argDenot` is not allowed to
          // consult short-lived things like the current constraint, so it has no other
          // choice. The problem will be healed later, when normal selection fails
          // and we try to instantiate type variables to compensate. But we have to make
          // sure we do not issue a type error before we get there.

The fix then was simply to catch a TypeError in isMatchedBy and return false instead. That would catch other TypeErrors as well (e.g. CyclicReference). Arguably that's OK. isMatchedBy is a low-level operation that should not itself issue errors and should fail in a safe way when it encounters a TypeError.

some more output to that message to see the current

@odersky
Copy link
Contributor Author

odersky commented Jul 14, 2022

The CB build fails because of this:

sbt.librarymanagement.ResolveException: Error downloading tools.jackson:jackson-base:3.0.0-SNAPSHOT
[error]   Not found
[error]   Not found
[error]   not found: /root/.ivy2/local/tools.jackson/jackson-base/3.0.0-SNAPSHOT/ivys/ivy.xml
[error]   not found: https://scala-webapps.epfl.ch/artifactory/sbt-plugin-releases/tools.jackson/jackson-base/3.0.0-SNAPSHOT/ivys/ivy.xml
[error]   download error: Caught java.io.IOException (Server returned HTTP response code: 409 for URL: https://scala-webapps.epfl.ch/artifactory/central/tools/jackson/jackson-base/3.0.0-SNAPSHOT/jackson-base-3.0.0-SNAPSHOT.pom) while downloading https://scala-webapps.epfl.ch/artifactory/central/tools/jackson/jackson-base/3.0.0-SNAPSHOT/jackson-base-3.0.0-SNAPSHOT.pom
[error]   not found: https://oss.sonatype.org/content/repositories/snapshots/tools/jackson/jackson-base/3.0.0-SNAPSHOT/jackson-base-3.0.0-SNAPSHOT.pom

@dwijnand
Copy link
Member

Now there's a real puzzler: how does catching a TypeError cause scala-webapps.epfl.ch to start returning 409s?! 😄

@odersky odersky requested a review from griggt July 14, 2022 15:00
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

This seems like a little extreme. Is there no way to add a guard somewhere that quietly returns false instead of recovering from a thrown exception? Or can we at least tighten around what call can't be guarded but we want to recover from it throwing?

@odersky
Copy link
Contributor Author

odersky commented Jul 14, 2022

We recover from TypeError in several other places as well, so this is nothing new.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Ah, I see, 24 matched lines of with "case" and "TypeError". OK, fair enough.

@griggt
Copy link
Contributor

griggt commented Jul 15, 2022

Rebased to fix community build failure.

Copy link
Contributor

@griggt griggt left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, I was struggling to make the connection.

@griggt griggt merged commit d247c37 into scala:main Jul 15, 2022
@griggt griggt deleted the fix-15673 branch July 15, 2022 04:45
@Kordyjan Kordyjan added the backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" label Jul 26, 2022
@Kordyjan Kordyjan added this to the 3.2.0 backports milestone Jul 26, 2022
@Kordyjan Kordyjan added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Jul 26, 2022
Kordyjan added a commit that referenced this pull request Jul 27, 2022
Backport #15675: Survive TypeErrors in isMatchedBy
@Kordyjan Kordyjan modified the milestones: 3.2.0 backports, 3.2.1 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported. how-i-fixed-it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad parameter reference CC[A]#C at typer
5 participants