Skip to content

Restrict implicit args to using #22458

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 1 commit into from
Mar 10, 2025

Conversation

som-snytt
Copy link
Contributor

Typer#adaptNoArgsImplicitMethod populates implicit args when an arg list is missing. To remedy missing implicits, it tries a named application using args it did find. Then Applications#tryDefault supplies a default arg if available. A previous fix to allow tryDefault to supply implicit args for implicit params is now restricted to explicit using; typer now adds using for implicit when it needs to try defaults.

This commit restores propagatedFailure and the previous condition that default params are tried if there is an error that is not an ambiguity. An additional restriction is that default params must be useful: there must be a param which has a default arg to be added (because it's not a named arg).

Fixes #22439

@som-snytt
Copy link
Contributor Author

Aspirationally,

Since issueErrors specifies endPos, the position used for
implicitArgTree by matchArgs is also tweaked to be the endPos
(of the appPos). That places a single caret in the familiar
spot following the existing application, instead of underlining.

That needs a mechanism for distinguishing user's using from typing for default args, where user expr requires underlining, but the latter case wants trailing caret.

@som-snytt som-snytt marked this pull request as ready for review January 26, 2025 10:32
@EugeneFlesselle
Copy link
Contributor

I'm in my exam period until 05/02, so I won't be able to review before then. I'll be happy to take a look once I can though.

@som-snytt som-snytt requested review from mbovel and removed request for EugeneFlesselle January 27, 2025 22:07
@som-snytt
Copy link
Contributor Author

@mbovel you were in this area last spring, and the LOC I restored will be familiar to you. (Maybe over-familiar.) If you wouldn't mind revisiting it for a review? This commit tweaks Eugene's previous fix by tweaking your previous fix.

@som-snytt som-snytt force-pushed the issue/22439-empty-implicit-app branch from 56a9d3b to 32c0f84 Compare February 2, 2025 01:00
@som-snytt som-snytt force-pushed the issue/22439-empty-implicit-app branch from 32c0f84 to f43543f Compare February 17, 2025 23:23
@Gedochao Gedochao requested a review from noti0na1 March 7, 2025 15:10
@Gedochao Gedochao assigned noti0na1 and unassigned mbovel Mar 7, 2025
@Gedochao Gedochao requested a review from sjrd March 7, 2025 15:41
Copy link
Member

@noti0na1 noti0na1 left a comment

Choose a reason for hiding this comment

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

LGTM. Very nice cleaning!

@som-snytt som-snytt force-pushed the issue/22439-empty-implicit-app branch 2 times, most recently from 2a8b6d8 to 24eac57 Compare March 7, 2025 23:06
@som-snytt
Copy link
Contributor Author

just a rebase

@som-snytt
Copy link
Contributor Author

Just realized that the new error is the fixed ticket: the code has spurious parens.

[error] 1575 |        val (types, values) = pats.map(evalPattern(scrutinee, _)).unzip()
[error]      |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]      |missing argument for parameter asPair of method unzip in trait StrictOptimizedIterableOps: (implicit asPair: ((dotty.tools.dotc.core.Types.Type, Objects.this.Value)) => (
[error]      |  A1, A2)): (List[A1], List[A2])
[error] one error found

@som-snytt som-snytt force-pushed the issue/22439-empty-implicit-app branch from 24eac57 to 2af66ef Compare March 8, 2025 19:51
@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 8, 2025

I appreciate the explanatory comment from January, but frankly it's not very meaningful to me right now in March.

OK, after reading it a couple of times, I kind of understand it.

Additionally, a reminder that it's always futile to update vulpix check files "by hand". The "div" line will be some width you did not anticipate (and not the width in your terminal window). It would be nice if there were a -Yvulpix mode to make it work.

Typer#adaptNoArgsImplicitMethod populates implicit args
when an arg list is missing. To remedy missing implicits,
it tries a named application `using` args it did find.
Then Applications#tryDefault supplies a default arg if
available. A previous fix to allow tryDefault to supply
implicit args for `implicit` params is now restricted
to explicit `using`; typer now adds `using` for `implicit`
when it needs to try defaults.

This commit restores propagatedFailure and the previous
condition that default params are tried if there is an error
that is not an ambiguity. An additional restriction is that
default params must be useful: there must be a param which
has a default arg to be added (because it's not a named arg).
@som-snytt som-snytt force-pushed the issue/22439-empty-implicit-app branch from 2af66ef to c37dc8b Compare March 8, 2025 20:11
@sjrd sjrd merged commit 798987a into scala:main Mar 10, 2025
29 checks passed
@tgodzik
Copy link
Contributor

tgodzik commented Mar 10, 2025

Any idea if it's safely portable to LTS? Or is it even needed to be back ported?

@som-snytt
Copy link
Contributor Author

@tgodzik I remember the code has history over the past 12 months, which suggests that it would be desirable on LTS. I can try it out and update this comment. (The delta is not trivial.)

@tgodzik
Copy link
Contributor

tgodzik commented Mar 10, 2025

I will try to backport it then if possible.

@som-snytt som-snytt deleted the issue/22439-empty-implicit-app branch March 10, 2025 15:47
@som-snytt
Copy link
Contributor Author

It cherry-picks, FWIW. The zip() error was introduced just a few days ago! Backporting is not fun: LTS doesn't have the LazyVals @nowarn to compile on JDK 23. Hats off to backporters.

@WojciechMazur WojciechMazur added this to the 3.7.0 milestone Mar 11, 2025
@tgodzik tgodzik added the release-notes Should be mentioned in the release notes label Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implicits are resolved with empty parameter lists
7 participants