Prevent opaque types leaking from transparent inline methods #23792
Conversation
|
Did you check the original issue example of #13461 ? I don't see it in the tests. |
|
Oh no, it's a macro :( . Sorry, for some reason multiple very different issues got merged into that one issue report, and I didn't notice how different the initial minimisation was. It doesn't work currently. I'll close this, but I want to see how open-community-build fares first |
jchyb
force-pushed
the
transparent-inline-opaque-overhaul
branch
from
04adb37 to
1b2c3fb
Compare
jchyb
force-pushed
the
transparent-inline-opaque-overhaul
branch
2 times, most recently
from
5a1fcb2 to
b960c0b
Compare
There was a problem hiding this comment.
I believe I have managed to fix all of the issues now mentioned in the issue ticket (thank you for notifying me about the missing one @soronpo). Open community build also seems to not show any new regressions introduced here. Below are some additional comments for the reviewer.
| else | ||
| typedExpr(rhs, tpt1.tpe.widenExpr)(using rhsCtx)) match | ||
| case typed @ Typed(outer, _) if typed.hasAttachment(InsertedTyped) => outer | ||
| case other => other |
There was a problem hiding this comment.
This part is connected to the i13461-a tests, where as an optimization the compiler added a Typed tree, which meant that we were not able to correctly handle result of the transparent tree, since we couldn't differentiate between Typed added by the compiler and Typed added by the user - the InsertedTyped exists for that purpose
| if forceCast then | ||
| // we need to force the cast for issues with ThisTypes, as ensureConforms will just | ||
| // check subtyping and then choose not to cast, leaving the previous, incorrect type | ||
| inlined.cast(target) |
There was a problem hiding this comment.
This, along with the withAdjustedThisTypes method, fixes the i13461-d test case. There, and Ident was typed as TermRef(ThisType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <root>)),module class <empty>)),Macro_1$package$)),MyOpaque$)),one), and was then Inlined into a place where those ThisTypes stopped being valid. This caused the implicit resolution to incorrectly be able to see the opaque type rhs. To fix this, we make sure we don't refer incorrectly to objects as ThisTypes with the help of withAdjustedThisTypes method
| @@ -0,0 +1,23 @@ | |||
| object Time: | |||
There was a problem hiding this comment.
This test-case was made by me, based on i13461-b.scala. to show and test the behaviors of nested transparent calls.
Thank YOU for finally getting this one fixed. I've been burned by issue on several occasions. |
jchyb
force-pushed
the
transparent-inline-opaque-overhaul
branch
from
b960c0b to
9ec814e
Compare
| * inserted by the Typer phase. We might want to remove it for the purpose of inlining, | ||
| * but only if it was not manually inserted by the user. | ||
| */ | ||
| private[typer] val InsertedTyped = new Property.Key[Unit] |
There was a problem hiding this comment.
Make it a StickyKey? (then it will survive copying)
There was a problem hiding this comment.
I see now that it's used in the same phase, so no need to make it a StickyKey.
|
|
||
| /** Map back all TermRefs that match the right element in `opaqueProxies` to the | ||
| * corresponding left element. | ||
| */ |
There was a problem hiding this comment.
It would be good to give an example showing the transformation on some code snippet.
| def thisTypeProxyExists = !thisProxy.isEmpty | ||
|
|
||
| // Unpacks `val ObjectDef$_this: ObjectDef.type = ObjectDef` reference back into ObjectDef reference | ||
| // For nested transparent inline calls, ObjectDef will be an another proxy, but that is okay |
There was a problem hiding this comment.
I did not understand the comment. Given this example, what type gets mapped to what other type?
There was a problem hiding this comment.
I've rewritten the comment from scratch (with an example), now it should be more clear what I meant there
Before this PR, every transparent inline call returning an opaque type
would actually be typed with an intersection type `DECLARED & ACTUAL`,
where `DECLARED` was the declared return type of the transparent method,
and `ACTUAL` was the type actually returned in the expansion, with every
opaque type alias then dealiased. There was no way to guard against this
dealiasing. With the changes in this PR, users are now able to manually
ensure that they receive the types they want, although they might have
to manually annotate the returned type inside of the transparent inline
method body (as described in the added documentation section).
The previous dealiasing was caused by the proxy mechanism in inlining,
which would effectively deals every opaque type, that is transparent
from the perspective of the original method declaration. Now, we try to
map the results of the transparent inline back to the original (opaque)
types.
However all of this is only true for the outermost transparent inline
method calls. Nested calls will not be affected by this change. This is
because the type checker in the original context of the method will see
the opaque type as transparent (so it will type the rest of the method
according to that), and that typing must still hold after inlining the
method e.g.:
```
object Time:
opaque type Time = String
transparent inline makeTime(): Time = "1h"
transparent inline listTime(): List[Time] = List[String](makeTime())
// mapping the results of makeTime() back into opaque types outside
// of the scope of Time will cause an inlining compilation error
// (which we are generally trying to avoid, and which would be
// considered a bug in the compiler).
```
This might cause the aliased type to still leak in a manner that may
feel unexpected. In the above example, even if the List does not have
an explicit type parameter, the type inference will still decide on
`String`, causing any call to listTime to leak that type. This is also
touched upon in the added docs.
This PR might cause some source/library incompatibilities connected to
the changed returned types (but I doubt it’s many, considering the
additional required effort of ignoring type inference if we want the
outputted type to be different).
Certain macros could return nodes typed with incorrect ThisTypes, which would reference module types outside of their scope. We remap those problematic nodes to TermRefs pointing to objects, and then possibly manually cast the returned node to the remapped type, as the ensureConforms method would just leave the previous incorrect type after confirming that the remapped type is a subtype of the previous incorrect one.
jchyb
force-pushed
the
transparent-inline-opaque-overhaul
branch
from
9ec814e to
e611110
Compare
|
Thank you @odersky for the review! I've adjusted the comments and added more examples - it should be better now |
4 participants
Fixes #13461
Before this PR, every transparent inline call returning an opaque type would actually be typed with an intersection type DECLARED & ACTUAL, where DECLARED was the declared return type of the transparent method, and ACTUAL was the type actually returned in the expansion, with every opaque type alias then dealiased. There was no way to guard against this dealiasing. With the changes in this PR, users are now able to manually ensure that they receive the types they want, although they might have to manually annotate the returned type inside of the transparent inline method body (as described in the added documentation section).
The previous dealiasing was caused by the proxy mechanism in inlining, which would effectively deals every opaque type, that is transparent from the perspective of the original method declaration. Now, we try to map the results of the transparent inline back to the original (opaque) types.
However all of this is only true for the outermost transparent inline method calls. Nested calls will not be affected by this change. This is because the type checker in the original context of the method will see the opaque type as transparent (so it will type the rest of the method according to that), and that typing must still hold after inlining the method e.g.:
This might cause the aliased type to still leak in a manner that may feel unexpected. In the above example, even if the List does not have an explicit type parameter, the type inference will still decide on
String, causing any call to listTime to leak that type. This is also touched upon in the added docs.This PR might cause some source/library incompatibilities connected to the changed returned types (but I doubt it’s many, considering the additional required effort of ignoring type inference if we want the outputted type to be different).