Skip to content

Refinements to skolemizaton #23513

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

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

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jul 12, 2025

  1. Deskolemize after fully defining type

    The order was the opposite before. That led to skolem types escaping in the constraint and then being installed in the inferred type of a val or def. See Unexpected union of skolems as Singleton type argument #23489 for a test case where this shows up.

  2. Re-use skolems generated for arguments.

Fixes #23489

@odersky odersky requested a review from mbovel July 12, 2025 16:07
odersky added 3 commits July 12, 2025 18:26
The order was the opposite before. That led to skolem types escaping in the constraint and then being
installed in the inferred type of a val or def.
There are at least two instances where we might skolemize the type of an argument.
One is in safeSubstParams when we compute a result type of a dependent method type
application. The other is in constrainIfDependentParamRef where we constrain type
variables representing dependent parameters.

This used to give several skolem types that ended up in a common OrType. We now
store computed skolem types in attachments of argument types and re-use them if
possible.
@odersky odersky changed the title Deskolemize after fully defining type Refinements to skolemizaton Jul 13, 2025
if arg == null then
SkolemType(argType)
else
arg.getAttachment(Skolemized) match
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't it happen that the same argument tree is shared among different application trees? In that case, wouldn't that be wrong to give them the same skolem type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's indeed a concern. We don't usually share trees but we don't have a guarantee here.

The new scheme works also if argument trees are not unique. Instead of storing
skolem types directly in attachment of arguments, we store a map from arguments
to their skolems in the enclosing application node. But we check then that the
same argument tree does not appear several times in an application. If it does
appear several times, we don't generate the map.

Furthermore, we are also safe if the whole application tree is used several times,
since for each call we generate new skolems and store them in the map. So different
calls get different skolems.
@odersky
Copy link
Contributor Author

odersky commented Jul 14, 2025

Commit message of last commit:

Refinement: Make scheme work also if arguments are duplicated

The new scheme works also if argument trees are not unique. Instead of storing
skolem types directly in attachment of arguments, we store a map from arguments
to their skolems in the enclosing application node. But we check then that the
same argument tree does not appear several times in an application. If it does
appear several times, we don't generate the map.

Furthermore, we are also safe if the whole application tree is used several times,
since for each call we generate new skolems and store them in the map. So different
calls get different skolems.

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.

Unexpected union of skolems as Singleton type argument
2 participants