Skip to content

Add support for @deprecatedName #19086

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 1 commit into
base: main
Choose a base branch
from
Open

Conversation

hamzaremmal
Copy link
Member

Closes #19077

@som-snytt
Copy link
Contributor

I won't attempt it, then! Thanks!

@hamzaremmal hamzaremmal force-pushed the i19077 branch 2 times, most recently from 40dda37 to 040e385 Compare December 9, 2023 18:57
@hamzaremmal hamzaremmal added the release-notes Should be mentioned in the release notes label Dec 9, 2023
@hamzaremmal hamzaremmal marked this pull request as ready for review December 9, 2023 19:05
@He-Pin
Copy link
Contributor

He-Pin commented Dec 18, 2023

Hope this be backported to scala 3.3.x too

@hamzaremmal
Copy link
Member Author

Hope this be backported to scala 3.3.x too

It will be

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.

The primary use case is the parameter name changed but you want to support the previous name. def f(@deprecatedName("b") cond: Boolean). Either name compiles, but one warns. If the annotation arg is absent, the semantics is don't use any name.

@hamzaremmal
Copy link
Member Author

FYI: I've implemented this, I'm just making sure that nothing is broken before I push it

@som-snytt som-snytt assigned hamzaremmal and unassigned som-snytt Feb 26, 2024
@hamzaremmal hamzaremmal force-pushed the i19077 branch 2 times, most recently from 4115910 to 6a35378 Compare March 11, 2024 13:39
if methodType.paramNames.contains(name) then
em"parameter $name of $methString is already instantiated"
else
em"$methString does not have a parameter $name"
Copy link
Contributor

@odersky odersky Mar 12, 2024

Choose a reason for hiding this comment

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

I had another idea (not what we discussed this afternoon). We could keep the logic as before but at this point, instead of failing, we do an addiitonal check. Say our argument is x = e. We check whether the method symbol has a deprecatedName("x") annotation on a parameter with new name y. If that's the case we change the NamedArg name to y and try again. If that succeeds with issue the deprecated name warning.

@som-snytt
Copy link
Contributor

Despite my previous assertion, I attempted it. #19086 (comment)

Likely superseded by #21588

Note that it's necessary to check named args fairly eagerly for deprecation, since naming itself can be deprecated.

I'm sure I adopted some idioms from this PR after reading it, but I don't remember if it hits the snafu I encountered where paramSymss fails when overload resolution is comparing second param list of a poly method (because param names on the symbol and param types on the method type are out of sync).

The first attempt on the other PR was (IIRC) the suggestion from March, to try alternatives just before failing. (I don't recall the test failure.) Since it has to check even a good primary name for deprecation, it's easy just to collect the annotations for the current param list, and check first for the primary name and then for the alt name (since otherwise it will fail somehow anyway).

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.

dotc ignores deprecatedName
4 participants