Skip to content

Warn when open modifier is redundant (traits, abstract classes and local classes) #11971

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
Apr 19, 2021

Conversation

megri
Copy link
Contributor

@megri megri commented Apr 1, 2021

#11963

A first draft to I make sure I'm on the right path. I'd like to establish that my thinking/coding style is correct as well. :)

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

I'd like to establish that my thinking/coding style is correct as well

Yeah that's fine, the only nitpick I have is that the first line of your commit message should be more descriptive (e.g. "Disallow open modifier when redundant"), in the body of the commit message you can then write Fixes #11963 (it needs to be in this format for the issue to be auto-closed by github when the PR is merged).

@@ -2505,3 +2505,10 @@ import transform.SymUtils._
|Inlining such definition would multiply this footprint for each call site.
|""".stripMargin
}

class OpenModifierRedundantForAbstractClassOrTrait(cls: untpd.TypeDef, isTrait: Boolean)(using Context)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of defining a new message class, it might be possible to generalize the existing ModifierRedundantForObjects class to a ModifierRedundant that can be used for non-classes too.

Copy link
Contributor Author

@megri megri Apr 1, 2021

Choose a reason for hiding this comment

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

I would need to disambiguate between abstract/inner classes, traits and objects. Should this be done on an untpd.Tree or is there a more suitable abstraction?

edit: maybe an union type of untpd.ModuleDef | untpd.TypeDef ?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the cls argument isn't used here and could just be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to keep the explain-method from ModifierRedundantForObjects around and generalise it. I could pass the information from the report site as a String but I think it'd richer information would be preferable, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I patched the functionality a bit. It would be nice if the recommended solution in explain kept the template (or lack thereof) around, but for large defs that would be spammy unless truncated.

@megri megri force-pushed the issue-11963 branch 5 times, most recently from e4bcccb to 1fcdc61 Compare April 2, 2021 09:38
@smarter
Copy link
Member

smarter commented Apr 2, 2021

(I'm taking some time off so I won't be available to review this PR this week or the next)

@megri megri marked this pull request as draft April 2, 2021 22:49
@odersky
Copy link
Contributor

odersky commented Apr 6, 2021

I fear it's overengineered. A fix like this should cost inside of 5 lines of code. Everything else should be discussed separately.

In my experience flag checking can easily explode in complexity which makes it unmaintainable. It needs to be reigned in by design from the start.

@megri
Copy link
Contributor Author

megri commented Apr 6, 2021

I fear it's overengineered. A fix like this should cost inside of 5 lines of code. Everything else should be discussed separately.

In my experience flag checking can easily explode in complexity which makes it unmaintainable. It needs to be reigned in by design from the start.

Fair enough. I'll remove the enum and reduce the surface area. My only remaining concern is how to keep the error explanation nice and relevant.

edit: I have pushed the changes

@megri megri force-pushed the issue-11963 branch 4 times, most recently from 9f89d7a to 406a370 Compare April 6, 2021 12:31
@odersky
Copy link
Contributor

odersky commented Apr 6, 2021

Flags checking is a bit of a mess, I am afraid, so I am not sure this was a good first issue after all.

In fact, all flags should be checked in Checking.checkWellformed. The flags checking code in Desugar should really be the exception. It has to do with the fact that original flags are added and suppressed by Desugaring, which means flags had to be checked before. But we should not generalize this.

@megri
Copy link
Contributor Author

megri commented Apr 6, 2021

Flags checking is a bit of a mess, I am afraid, so I am not sure this was a good first issue after all.

In fact, all flags should be checked in Checking.checkWellformed. The flags checking code in Desugar should really be the exception. It has to do with the fact that original flags are added and suppressed by Desugaring, which means flags had to be checked before. But we should not generalize this.

The original intention was to give a warning in this case, but it seems checkWellFormed raises errors. Does this change the story or the intended location of the code?

Having come this far, unless there's a desire to have someone more experienced handle this, I'd like to continue working on the issue.

@odersky
Copy link
Contributor

odersky commented Apr 7, 2021

The original intention was to give a warning in this case, but it seems checkWellFormed raises errors. Does this change the story or the intended location of the code?

No. checkWellFormed can also do warnings.

Having come this far, unless there's a desire to have someone more experienced handle this, I'd like to continue working on the issue.

OK, great! Since I was looking at the code I have done some refactorings and moved the module checking code from Desugar to Checking, so that others will not be tempted to add to checking code in Desugar. So you'll need to rebase.

@megri
Copy link
Contributor Author

megri commented Apr 7, 2021

The original intention was to give a warning in this case, but it seems checkWellFormed raises errors. Does this change the story or the intended location of the code?

No. checkWellFormed can also do warnings.

Having come this far, unless there's a desire to have someone more experienced handle this, I'd like to continue working on the issue.

OK, great! Since I was looking at the code I have done some refactorings and moved the module checking code from Desugar to Checking, so that others will not be tempted to add to checking code in Desugar. So you'll need to rebase.

Great, thanks :)

@megri megri marked this pull request as ready for review April 7, 2021 20:09
@megri megri changed the title fix issue 11963 Warn when open modifier is redundant (traits, abstract classes and local classes) Apr 7, 2021
@megri
Copy link
Contributor Author

megri commented Apr 9, 2021

I think this is ready for final review

warn(RedundantModifier(Open))
if sym.isAllOf(Abstract | Open) then
warn(RedundantModifier(Open))
if sym.is(Open) && sym.owner.is(Method) then
Copy link
Member

Choose a reason for hiding this comment

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

This can be generalized to:

Suggested change
if sym.is(Open) && sym.owner.is(Method) then
if sym.is(Open) && sym.isLocal then

To also include val owners, local class owner, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@smarter smarter enabled auto-merge April 19, 2021 08:39
@smarter smarter merged commit dede682 into scala:master Apr 19, 2021
@Kordyjan Kordyjan added this to the 3.0.1 milestone Aug 2, 2023
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.

4 participants