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.

Sign up for GitHub

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

Jump to bottom

Followup fix to transparent inline conversion #18130

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Followup fix to transparent inline conversion #18130

merged 1 commit into from
Jul 10, 2023

Conversation

julienrf
Copy link
Contributor

@julienrf julienrf commented Jul 4, 2023 •
edited by dwijnand
Loading

Fixes #18123
Follow-up of #17924

@julienrf julienrf linked an issue Jul 4, 2023 that may be closed by this pull request
Regression in implicit resoultion for arguments passed by name #18123
Closed
@julienrf julienrf added this to the 3.3.2 milestone Jul 4, 2023
@julienrf julienrf requested a review from odersky July 4, 2023 10:14
@julienrf julienrf mentioned this pull request Jul 4, 2023
Closed
@dwijnand dwijnand changed the title Fixes #18123 Followup fix to transparent inline conversion Jul 4, 2023
@odersky
Copy link
Contributor

odersky commented Jul 4, 2023

It's a super weird program.

The problem is the rep extension method. If that method is transparent inline, it works. If it is just inline, it worked before #17924 but does not work now. If it is not inline it does not work. The error is in each case the same.

So I think these programs really worked by accident.

If more type variables are instantiated that can be good or bad for implicit search

  • good: fewer ambiguities
  • bad: some cases are thrown out that could have passed if we had let type type variable open.

So, there is no clear answer whether we should constrain more or not. It depends on the circumstances.

But the point is, #17924 was the correct fix relative to the rationale given in the comment of constrainResult. But it seems there's software out that exploited the gap. The fix in the PR is maybe the best one can do. Or else go back to kittens and the other program and tell them to use transparent inline for some of their methods.

@odersky odersky assigned julienrf and unassigned odersky Jul 4, 2023
julienrf added a commit to scalacenter/weapon-regex that referenced this pull request Jul 5, 2023
@julienrf
Explicitly upcast intermediate result to aid program elaboration
7577c29 
See scala/scala3#18130 for context.

We add an explicit type ascription to help the compiler infer contextual arguments.
@julienrf julienrf mentioned this pull request Jul 5, 2023
Merged
@julienrf
Copy link
Contributor Author

julienrf commented Jul 5, 2023

Or else go back to kittens and the other program and tell them to use transparent inline for some of their methods.

I am considering this option.

What is the impact of making transparent an inline method that takes given parameters of type NotGiven[Something]? Is this going to cancel the NotGiven guarantee (because wildApprox is used)?

@odersky
Copy link
Contributor

odersky commented Jul 5, 2023

No idea. Needs to be tried out.

julienrf added a commit to scalacenter/kittens that referenced this pull request Jul 5, 2023
@julienrf
Future-proof auto implementation
b9eee34 
For context see scala/scala3#18130.

We put fewer constraints to program elaboration by making some methods `transparent`.
@julienrf julienrf mentioned this pull request Jul 5, 2023
Closed
@julienrf
Copy link
Contributor Author

julienrf commented Jul 5, 2023 •
edited
Loading

I’ve submitted the following "fixes" to the affected projects:

stryker-mutator/weapon-regex#230
typelevel/kittens#582

I would rather see those projects fixed instead of having nonsensical code in the compiler. Does someone have a different opinion?

@julienrf julienrf marked this pull request as draft July 6, 2023 08:31
julienrf added a commit to scalacenter/dotty that referenced this pull request Jul 6, 2023
@julienrf
Adjust the fix for scala#9685 to increase source compatibility.
8345be4 
We apply the full fix under `-source future` only, and a partial fix otherwise.

This is a followup of scala#17924 that fixes the source incompatibilities reported in scala#18130.

We test that the behavior under `-source future` still fixes scala#9685, and that without `-source future` we fix both scala#9685 and scala#18130.
@julienrf julienrf force-pushed the fix-18123 branch 3 times, most recently from 0090dd1 to 6eddd57 Compare July 6, 2023 14:22
@julienrf
Adjust the fix for scala#9685 to increase source compatibility.
76ca3e6 
We apply the full fix under `-source future` only, and a partial fix otherwise.

This is a followup of scala#17924 that fixes the source incompatibilities reported in scala#18123.

We test that the behavior under `-source future` still fixes scala#9685, and that without `-source future` we fix both scala#9685 and scala#18123.
@julienrf julienrf marked this pull request as ready for review July 6, 2023 20:32
@julienrf
Copy link
Contributor Author

julienrf commented Jul 6, 2023

I took a different approach. Since there is no evidence we should revert what was done in #17924, I've adjusted the fix to still resolve the original issue #9685 but without causing the source incompatibility reported in #18123. The behavior originally implemented in #17924 is still present under -source future.

@julienrf julienrf requested a review from bishabosha July 6, 2023 20:36
@julienrf julienrf assigned bishabosha and unassigned julienrf Jul 6, 2023
julienrf added a commit to scalacenter/weapon-regex that referenced this pull request Jul 10, 2023
@julienrf
Explicitly upcast intermediate result to aid program elaboration
af84a81 
See scala/scala3#18130 for context.

We add an explicit type ascription to help the compiler infer contextual arguments.
hugo-vrijswijk pushed a commit to stryker-mutator/weapon-regex that referenced this pull request Jul 10, 2023
@julienrf
Explicitly upcast intermediate result to aid program elaboration (#230)
3204411 
See scala/scala3#18130 for context.

We add an explicit type ascription to help the compiler infer contextual arguments.
@bishabosha bishabosha merged commit 3de184c into scala:main Jul 10, 2023
@julienrf julienrf mentioned this pull request Jul 10, 2023
Closed
@julienrf julienrf deleted the fix-18123 branch July 10, 2023 08:55
@Kordyjan Kordyjan modified the milestones: 3.3.2, 3.4.0 Aug 1, 2023
@julienrf julienrf mentioned this pull request Dec 20, 2023
Merged
@nicolasstucki nicolasstucki mentioned this pull request Jan 24, 2024
Closed
nicolasstucki added a commit to dotty-staging/dotty that referenced this pull request Mar 11, 2024
@nicolasstucki
Revert "Followup fix to transparent inline conversion (scala#18130)"
2421581 
This reverts commit 3de184c.
@nicolasstucki nicolasstucki mentioned this pull request Mar 12, 2024
Merged
nicolasstucki added a commit that referenced this pull request Mar 12, 2024
@nicolasstucki
Revert broken changes with transparent inline (#19922)
f663a8b 
Proposed in
#19415 (comment)

Fixes #19415

Reverts #18130 and
#17924.

 #18130 is still closed as it was introduced by #17924.

This reopens #9685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bishabosha bishabosha bishabosha approved these changes

@odersky odersky odersky approved these changes

Assignees

@bishabosha bishabosha

Labels
None yet
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.

Regression in implicit resoultion for arguments passed by name
4 participants
@julienrf @odersky @bishabosha @Kordyjan