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 #20458: do not expose ClassInfo in quotes reflect widenTermRefByName #20468

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented May 24, 2024

Previously ClassInfo could easily be exposed with calls like TypeRepr.of[T].termRef.widenTermRefByName.

Fixes #20458

Previously ClassInfo could be easily be exposed with calls like
`TypeRepr.of[T].termRef.widenTermRefByName`.
@jchyb jchyb requested a review from smarter May 27, 2024 13:08
@jchyb jchyb changed the title Fix #20458: do not expose ClassInfo in widenTermRefByName Fix #20458: do not expose ClassInfo in quotes reflect widenTermRefByName May 27, 2024
@jchyb jchyb requested a review from hamzaremmal June 17, 2024 09:58
@hamzaremmal hamzaremmal merged commit 3d16f33 into scala:main Jul 1, 2024
19 checks passed
@hamzaremmal hamzaremmal deleted the fix-i20458 branch July 1, 2024 10:36
@dwijnand
Copy link
Member

dwijnand commented Jul 1, 2024

This doesn't seem like the right fix. The term reference for the Any companion object has an underlying Any companion module class (with nothing interesting). Widening the term to that class info is correct.

The original issue just seems invalid. Don't call asType on a term type.

Perhaps the user wanted to call .namedType, which might be a useful addition to the Quotes API:

    def namedType(using Context): NamedType =
      if (isType) typeRef else termRef

@jchyb
Copy link
Contributor Author

jchyb commented Jul 1, 2024

I see where you are coming from, but there is a conscious effort in the design of the quotes reflect API to never expose ClassInfo to users. At this point, there are no tools there for users to be able to work with ClassInfo. Here, since ClassInfo is not supported, we will instead return a TermRef reference to a symbol of that class (which users can work on and analyze with the tools provided to them).

Additionally, calling any.termRef (like in the minimization) will give us TermRef(ThisType(TypeRef(NoPrefix,module class scala)),class Any), any.termRef.asType will also work, as TermRef is a TypeRepr (in the quotes reflect API), which can be converted into a Type. The issue here is specifically about the unexpected ClassInfo crashing the compiler, as it was not supposed to be exposed here. Even outside of that I really do not think we should at any point allow to return an incorrect type from a quotes reflect API method (widenTermRefByName is supposed to return TypeRepr, but ClassInfo is not that).

About the original issue minimization itself, usually with the typeRepr.widen methods (like widen, widenByName, and widenTermRefByName), we would expect them to either widen, or if that is not possible, stay the same. Because of this, they can be a little overused, like what I imagine happened in the original issue minimisation (after this fix, in practice, the type stays the same - TermRef(ThisType(TypeRef(NoPrefix,module class scala)),class Any), since it is not possible to widen it in the quotes reflect API).

@dwijnand
Copy link
Member

dwijnand commented Jul 1, 2024

In processing case _: Any I'm not sure why you want to end up with the term reference to the Any object. The user code just doesn't seem right to me.

The issue here is specifically about the unexpected ClassInfo crashing the compiler, as it was not supposed to be exposed here. Even outside of that I really do not think we should at any point allow to return an incorrect type from a quotes reflect API method (widenTermRefByName is supposed to return TypeRepr, but ClassInfo is not that).

I could get behind something like that, but that's not what your patch is doing.

@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
WojciechMazur added a commit that referenced this pull request Jul 10, 2024
…TermRefByName" to LTS (#21152)

Backports #20468 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
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.

Pattern term of CaseDef is an internal ClassInfo that crashes when matched on
4 participants