Skip to content

[SPARK-16877][BUILD] Add rules for preventing to use Java annotations (Deprecated and Override) #14490

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

Closed
wants to merge 1 commit into from

Conversation

HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Aug 4, 2016

What changes were proposed in this pull request?

This PR adds both rules for preventing to use @Deprecated and @Override.

  • Java's @Override
    It seems Scala compiler just ignores this. Apparently, override modifier is only mandatory for " that override some other concrete member definition in a parent class" but not for for incomplete member definition (such as ones from trait or abstract), see (http://www.scala-lang.org/files/archive/spec/2.11/05-classes-and-objects.html#override)

    For a simple example,

    • Normal class - needs override modifier
    scala> class A { def say = {}}
    defined class A
    
    scala> class B extends A { def say = {}}
    <console>:8: error: overriding method say in class A of type => Unit;
    method say needs `override' modifier
         class B extends A { def say = {}}
                                 ^
    • Trait - does not need override modifier
    scala> trait A { def say }
    defined trait A
    
    scala> class B extends A { def say = {}}
    defined class B

    To cut this short, this case below is possible,

    scala> class B extends A { 
         |    @Override
         |    def say = {}
         | }
    defined class B

    we can write @Override annotation (meaning nothing) which might confuse engineers that Java's annotation is working fine. It might be great if we prevent those potential confusion.

  • Java's @Deprecated
    When @Deprecated is used, it seems Scala compiler recognises this correctly but it seems we use Scala one @deprecated across codebase.

How was this patch tested?

Manually tested, by inserting both @Override and @Deprecated. This will shows the error messages as below:

Scalastyle checks failed at following occurrences:
[error] ... : @deprecated should be used instead of @java.lang.Deprecated.
Scalastyle checks failed at following occurrences:
[error] ... : override modifier should be used instead of @java.lang.Override.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Aug 4, 2016

cc @srowen and @holdenk as well.

@rxin
Copy link
Contributor

rxin commented Aug 4, 2016

LGTM pending Jenkins.

@@ -250,6 +250,14 @@ This file is divided into 3 sections:
<customMessage>Omit braces in case clauses.</customMessage>
</check>

<!-- SPARK-16877: Avoid Java annotations -->
<check customId="OverrideJavaCase" level="error" class="org.scalastyle.scalariform.TokenChecker" enabled="true">
<parameters><parameter name="regex">^Override$</parameter></parameters>
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to look for a leading "@" as well?

Copy link
Member Author

@HyukjinKwon HyukjinKwon Aug 4, 2016

Choose a reason for hiding this comment

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

I actually tried ^@Override$ first but I found this Scala checking recognising this token as just Override.

Copy link
Member

Choose a reason for hiding this comment

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

Hm so this matches "@OverRide" but not "Override" now, but would reverse if the regex included "@"? That sounds flipped. @ isn't a special char. You're doubly sure that's right? It also seems like this is matching "Override" on a line alone but should be looking for "@OverRide" anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, actually I tried RegexChecker as well to grep this case but then I found actually I should come up with a complex regular expression for some exceptional cases such as

  • @Override in comments
  /**
...
   *    @Override
   *    public void close(Throwable errorOrNull) {
   *      // close the connection
   *    }
...
  • @Override in codegen
...
"  @Override public String toString() { return \"" + toStringValue + "\"; }}"
...

So, I had to use TokenChecker.

Copy link
Contributor

@holdenk holdenk Aug 4, 2016

Choose a reason for hiding this comment

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

I just reprored this locally on my machine as well (doesn't trigger with @Override or \@Override as the regex rule). One guess is the token checker has split them into separate tokens perhaps?

Copy link
Member Author

@HyukjinKwon HyukjinKwon Aug 4, 2016

Choose a reason for hiding this comment

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

Yes, I just ran a test after cloning scalariform which scalastyle uses as lexer as below:

ScalaLexer.rawTokenise("@Override")

It seems this becomes

2016-08-04 1 29 11

different tokens.

(BTW, maybe we should avoid to write @Override as it is.. I started to feel guilty for cc him/her here and there....)

@SparkQA
Copy link

SparkQA commented Aug 4, 2016

Test build #63201 has finished for PR 14490 at commit 79eaef7.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Aug 4, 2016

Test build #63207 has finished for PR 14490 at commit 79eaef7.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Aug 4, 2016

Test build #63215 has finished for PR 14490 at commit 79eaef7.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

asfgit pushed a commit that referenced this pull request Aug 4, 2016
… (Deprecated and Override)

## What changes were proposed in this pull request?

This PR adds both rules for preventing to use `Deprecated` and `Override`.

- Java's `Override`
  It seems Scala compiler just ignores this. Apparently, `override` modifier is only mandatory for " that override some other **concrete member definition** in a parent class" but not for for **incomplete member definition** (such as ones from trait or abstract), see (http://www.scala-lang.org/files/archive/spec/2.11/05-classes-and-objects.html#override)

  For a simple example,

  - Normal class - needs `override` modifier

  ```bash
  scala> class A { def say = {}}
  defined class A

  scala> class B extends A { def say = {}}
  <console>:8: error: overriding method say in class A of type => Unit;
   method say needs `override' modifier
         class B extends A { def say = {}}
                                 ^
  ```

  - Trait - does not need `override` modifier

  ```bash
  scala> trait A { def say }
  defined trait A

  scala> class B extends A { def say = {}}
  defined class B
  ```

  To cut this short, this case below is possible,

  ```bash
  scala> class B extends A {
       |    Override
       |    def say = {}
       | }
  defined class B
  ```
  we can write `Override` annotation (meaning nothing) which might confuse engineers that Java's annotation is working fine. It might be great if we prevent those potential confusion.

- Java's `Deprecated`
  When `Deprecated` is used,  it seems Scala compiler recognises this correctly but it seems we use Scala one `deprecated` across codebase.

## How was this patch tested?

Manually tested, by inserting both `Override` and `Deprecated`. This will shows the error messages as below:

```bash
Scalastyle checks failed at following occurrences:
[error] ... : deprecated should be used instead of java.lang.Deprecated.
```

```basg
Scalastyle checks failed at following occurrences:
[error] ... : override modifier should be used instead of java.lang.Override.
```

Author: hyukjinkwon <gurwls223@gmail.com>

Closes #14490 from HyukjinKwon/SPARK-16877.

(cherry picked from commit 1d78157)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@srowen
Copy link
Member

srowen commented Aug 4, 2016

Merged to master, 2.0

@asfgit asfgit closed this in 1d78157 Aug 4, 2016
@HyukjinKwon HyukjinKwon deleted the SPARK-16877 branch January 2, 2018 03:39
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.

5 participants