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

Repeated params must correspond in override #16836

Merged
merged 2 commits into from
Jun 26, 2023
Merged

Repeated params must correspond in override #16836

merged 2 commits into from
Jun 26, 2023

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Feb 6, 2023 •
edited
Loading

Refchecks runs after elimRepeated and did not
error on an attempt to override RepeatedParam with Seq.

Also show RepeatedParam in error message for double definition.

Fixes #12662

Resubmits #13248 which was opened against old master.

@som-snytt som-snytt marked this pull request as ready for review February 9, 2023 16:38
odersky
odersky requested changes Feb 12, 2023
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise looks good.

compiler/src/dotty/tools/dotc/typer/RefChecks.scala Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Decorators.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala Outdated Show resolved Hide resolved
@som-snytt
Copy link
Contributor Author

Thanks again for fruitful review. I spent some time to learn again what the moving parts are, but it still feels awkward to do this narrow check in these two spots, namely for matching methods and double definitions.

@som-snytt
Copy link
Contributor Author

tests/neg/override-scala2-macro was the other error message that showed Seq[A] instead of A*. It's an edge case which is left untouched now.

@som-snytt
Repeated params must correspond in override
3fad1e6 
Refchecks runs after elimRepeated and did not
error on an attempt to override RepeatedParam with
Seq. Also show RepeatedParam in error message.
@som-snytt
Only check last position, use DoubleDefinition
d6fd2c4
@som-snytt som-snytt force-pushed the issue/12662-varargs-override branch from 010bf8e to d6fd2c4 Compare April 19, 2023 16:51
@ckipp01 ckipp01 requested a review from odersky May 9, 2023 17:40
odersky
odersky approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Sorry for getting to this only now. I did not realize the ball was back in my court here!

Everything LGTM now.

@odersky odersky merged commit 9009f16 into scala:main Jun 26, 2023
@som-snytt som-snytt deleted the issue/12662-varargs-override branch June 26, 2023 14:02
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
@WojciechMazur WojciechMazur mentioned this pull request Nov 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

Assignees

@odersky odersky

Labels
None yet
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.

Varargs override with Seq should clash more
3 participants
@som-snytt @odersky @Kordyjan