Skip to content

Use more context for implicit search only if no default argument #23664

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

Merged
merged 3 commits into from
Aug 9, 2025

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 5, 2025

After an "implicit not found", we type additional arguments to get more context which might
give a larger implicit scope to search. With this commit we do that only if there is no
default argument for the implicit.

Based on #23659

This might fix #23610

@WojciechMazur
Copy link
Contributor

Both erikerlandson/coulomb and zio-archive/zio-connect are still failing in the OpenCB with these changes with the same errors as before, but maybe resolving issue mentioned in #23659 (comment) can hopefully fix it.

odersky added 3 commits August 6, 2025 17:40
We now use a blend of the new scheme and backwards compatible special case if type variables
as ClassTag arguments are constrained by further type variables.

Fixes scala#23611
After an "implicit not found", we type additional arguments to get more context which might
give a larger implicit scope to search. With this commit we do that only if there is no
default argument for the implicit.

This might fix scala#23610
@odersky
Copy link
Contributor Author

odersky commented Aug 6, 2025

I think there's nothing more I can do to make coulomb compile.

@odersky
Copy link
Contributor Author

odersky commented Aug 7, 2025

@som-snytt Since you recently worked in that corner, do you want to take a look for a review?

Copy link
Contributor

@EugeneFlesselle EugeneFlesselle left a comment

Choose a reason for hiding this comment

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

The changes seem reasonable to minimize impact from 45f7ef6 generally speaking.

For coulomb, I'm wondering if we should try returning with the deepened proto for AmbiguousImplicits even if constrainResult fails when applied to it? I opened #23687 to try it out.

@som-snytt
Copy link
Contributor

LGTM! as they say blithely.

With the caveat that I'm only partly up to speed, I wonder if tightening constraints would work better when the named application is subsequently typed. When defaults are tried, you have the opportunity for additional shenanigans (better constraints for certain failed args that canProfit) possibly without over-constraining successful args (or whatever causes problems).

I'm suspicious of an inference decision depending on whether I have a default arg, since people are already confused about that competition between givens and defaults.

I don't know anything about the mechanics of constraints; and I haven't yet understood concretely what the ctx.typerState.resetTo(saved) does right before the named application is tried to jog the defaults. I know removing it makes the test on my other ticket work and nothing breaks. (In that same area of ignorance, I don't know what ignored proto means or why removing the branch at the end of adaptNoArgsImplicitMethod doesn't break its test.)

On the Pekko ticket, minimal test is at https://github.com/scala/scala3/pull/23556/files and only involves a couple of givens. At bottom, inference breaks when the default is used, where the context bound is in the same param list.

implicit final def marshaller[A: Encoder](implicit printer: Printer = Printer.noSpaces): ToEntityMarshaller[A] =

TIL Pekko is a "Tower of Hanoi" of implicit conversions; with some debug, maybe I'll see what it's doing when the typer reset makes it forget what A is.

@som-snytt
Copy link
Contributor

I cloned coulomb this morning; I'll take another look if it stays broken in a way that is not terribly urgent.

Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

Just on the default arg check, I expressed dubiousness. But not regressing is also nice. I will have different opinions when I'm less ignorant.

@odersky
Copy link
Contributor Author

odersky commented Aug 9, 2025

TIL Pekko is a "Tower of Hanoi" of implicit conversions; with some debug, maybe I'll see what it's doing when the typer reset makes it forget what A is.

The original change in #23532 does change where implicit conversions are inserted. Any propagation of info from the result into a typing does that since it implicitly assumes that the info is not invalidated by an implicit conversion. So I am not terribly surprised that Pekko is affected.

@odersky
Copy link
Contributor Author

odersky commented Aug 9, 2025

@som-snytt, @odersky Can one of you approve, then we can merge this.

@odersky odersky merged commit 53c7472 into scala:main Aug 9, 2025
46 checks passed
@odersky odersky deleted the fix-23610 branch August 9, 2025 16:31
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.

Regression in typer for erikerlandson/coulomb
4 participants