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 #21669: Check parents non-empty before calling reduceLeft #21673

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

noti0na1
Copy link
Member

@noti0na1 noti0na1 commented Sep 30, 2024

Fix #21669

This part of code is from #20084

We should check parents is non-empty before calling reduceLeft.

I haven't been able to create a standalone test. To test locally:

i21669.scala

//> using dep "com.softwaremill.sttp.tapir::tapir-sttp-client:1.11.5"

import sttp.tapir.*
import sttp.tapir.client.sttp.SttpClientInterpreter

@main def run =
    lazy val pingGET = endpoint.get
        .in("ping")
        .out(stringBody)

    SttpClientInterpreter()
        .toRequest(pingGET, Some(uri"http://localhost:8080"))
> sbt publishLocal
> scala compile --server=false -S 3.6.0-RC1-bin-SNAPSHOT i21669.scala
-- [E008] Not Found Error: /dotty/i21669.scala:12:33 ------
12 |        .toRequest(pingGET, Some(uri"http://localhost:8080"))
   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |value uri is not a member of StringContext, but could be made available as an extension method.
   |
   |One of the following imports might fix the problem:
   |
   |  import sttp.client3.UriContext
   |  import sttp.client3.quick.UriContext
   |  import sttp.model.Uri.UriContext
   |
1 error found
Compilation failed

@noti0na1
Copy link
Member Author

noti0na1 commented Sep 30, 2024

Hi @odersky @EugeneFlesselle , I saw you worked on #20084. Do you think an empty parents is a valid case?

We also have many other places where we call parents.reduceLeft directly, do you think we need to concern these places as well? I guess these places are ok because the types must be classes/enums.

@noti0na1 noti0na1 requested a review from odersky October 3, 2024 13:41
@noti0na1 noti0na1 marked this pull request as ready for review October 3, 2024 13:41
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.

The other thing we could do is harden import sugestions. We catch NonFatal when computing a suggestion. We could also do the same when comparing suggestions. That's where the previous error happens.

@@ -1962,7 +1962,9 @@ trait Applications extends Compatibility {

def widenPrefix(alt: TermRef): Type = alt.prefix.widen match
case pre: (TypeRef | ThisType) if pre.typeSymbol.is(Module) =>
pre.parents.reduceLeft(TypeComparer.andType(_, _))
val ps = pre.parents
if ps.isEmpty then pre
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it's an invariant that parents of modules are non-empty. except if the module is a package. We should not ignore that invariant here. So before returning pre, let's add the assertion

assert(pre.typeSymbol.is(Package), pre)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, that's an invariant of if the module has been entered. But it could be wrong before. Anyway, it's an important invariant so I think we should not ignore it here, and catch any errors in importSuggestions instead.

@odersky odersky assigned noti0na1 and unassigned odersky Oct 3, 2024
@noti0na1
Copy link
Member Author

noti0na1 commented Oct 3, 2024

Hi @WojciechMazur , since we don't have a regression test currently, can you help testing this PR on CB to double check that it doesn't break anything? Thanks!

@WojciechMazur
Copy link
Contributor

We haven't found any new regressions in the OpenCB when compared with 3.6.0-RC1-bin-20241002-c332766-NIGHTLY

@odersky
Copy link
Contributor

odersky commented Oct 4, 2024

Does the PR fix the original problem reported in #21669?

@noti0na1
Copy link
Member Author

noti0na1 commented Oct 4, 2024

Does the PR fix the original problem reported in #21669?

Yes, I have tested it locally following the step I described.

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.

Scala 3.5.x compiler crash with java.lang.UnsupportedOperationException: empty.reduceLeft
3 participants