Skip to content

Enforce some linting#6964

Merged
dwijnand merged 2 commits intoscala:2.13.xfrom
som-snytt:issue/enforcement
Jul 31, 2018
Merged

Enforce some linting#6964
dwijnand merged 2 commits intoscala:2.13.xfrom
som-snytt:issue/enforcement

Conversation

@som-snytt
Copy link
Contributor

Enforce some linting

People keep commiting unused imports, which are
the easiest to check, so create some noise if they do that.

Mitigate the warnings; other warnings are mitigated in other PRs.

@scala-jenkins scala-jenkins added this to the 2.13.0-M5 milestone Jul 24, 2018
People keep commiting unused imports, which are
the easiest to check, so create some noise if they
do that.
@som-snytt som-snytt force-pushed the issue/enforcement branch from 740bdda to 34f91a2 Compare July 24, 2018 17:24
@som-snytt
Copy link
Contributor Author

Probably there's a better way to exclude some linting from junit subproject (where some tests exploit def testMe: Unit, but you surely want to warn about unused variables, which look like untested variables) and test (where partests can be quite junky).

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Thanks Andrew.

@dwijnand dwijnand merged commit 4bd5bf5 into scala:2.13.x Jul 31, 2018
@retronym
Copy link
Member

retronym commented Aug 2, 2018

@retronym
Copy link
Member

retronym commented Aug 2, 2018

Ah, the problem is that our setupPublishCore command that reloads the project with the optimizer enabled doesn't seem to compose with scalacOptions set in build.sbt.

Before:

> show compiler/scalacOptions
[info] *
[success] Total time: 0 s, completed 02/08/2018 12:21:35 PM
> setupPublishCore
[info] *** Welcome to the sbt build definition for Scala! ***
[info] Check README.md for more information.
> show compiler/scalacOptions
[info] * -opt:l:inline
[info] * -opt-inline-from:scala/**
[success] Total time: 0 s, completed 02/08/2018 12:21:44 PM

After:

> show compiler/scalacOptions
[info] * -Xlint:-nullary-override,-inaccessible,_
[success] Total time: 0 s, completed 02/08/2018 12:20:25 PM
> setupPublishCore
[info] *** Welcome to the sbt build definition for Scala! ***
[info] Check README.md for more information.
> show compiler/scalacOptions
[info] * -Xlint:-nullary-override,-inaccessible,_
[success] Total time: 0 s, completed 02/08/2018 12:20:31 PM

retronym added a commit to retronym/scala that referenced this pull request Aug 2, 2018
Regressed in scala#6964

Before:

```
$ sbt setupPublishCore 'show library/scalacOptions' 'show reflect/scalacOptions' 'show compiler/test:scalacOptions' 'show test/scalacOptions'
[info] Loading global plugins from /Users/jz/.sbt/0.13/plugins
[info] Loading project definition from /Users/jz/code/scala/project/project
[info] Loading project definition from /Users/jz/code/scala/project
[info] Resolving key references (11393 settings) ...
[info] *** Welcome to the sbt build definition for Scala! ***
[info] Check README.md for more information.
[info] *** Welcome to the sbt build definition for Scala! ***
[info] Check README.md for more information.
[info] * -Xlint:-nullary-override,-inaccessible,_
[success] Total time: 0 s, completed 02/08/2018 12:33:38 PM
[info] * -Xlint:-nullary-override,-inaccessible,_
[success] Total time: 0 s, completed 02/08/2018 12:33:38 PM
[info] * -Xlint:-nullary-override,-inaccessible,_
[success] Total time: 0 s, completed 02/08/2018 12:33:38 PM
[info] *
[success] Total time: 0 s, completed 02/08/2018 12:33:38 PM
```

After:

```
$ sbt setupPublishCore 'show library/scalacOptions' 'show reflect/scalacOptions' 'show compiler/test:scalacOptions' 'show test/scalacOptions'
[info] Loading global plugins from /Users/jz/.sbt/0.13/plugins
[info] Loading project definition from /Users/jz/code/scala/project/project
[info] Loading project definition from /Users/jz/code/scala/project
[info] Resolving key references (11393 settings) ...
[info] *** Welcome to the sbt build definition for Scala! ***
[info] Check README.md for more information.
[info] *** Welcome to the sbt build definition for Scala! ***
[info] Check README.md for more information.
[info] * -opt:l:inline
[info] * -opt-inline-from:scala/**
[info] * -Xlint:-nullary-override,-inaccessible,_
[info] * -sourcepath
[info] * /Users/jz/code/scala/src/library
[success] Total time: 0 s, completed 02/08/2018 12:31:18 PM
[info] * -opt:l:inline
[info] * -opt-inline-from:scala/**
[info] * -Xlint:-nullary-override,-inaccessible,_
[success] Total time: 0 s, completed 02/08/2018 12:31:18 PM
[info] * -opt:l:inline
[info] * -opt-inline-from:scala/**
[info] * -Xlint:-nullary-override,-inaccessible,_
[success] Total time: 0 s, completed 02/08/2018 12:31:18 PM
[info] * -opt:l:inline
[info] * -opt-inline-from:scala/**
[success] Total time: 0 s, completed 02/08/2018 12:31:18 PM
```
@retronym
Copy link
Member

retronym commented Aug 2, 2018

#6997 6997

@som-snytt som-snytt deleted the issue/enforcement branch August 2, 2018 09:47
@szeiger
Copy link
Contributor

szeiger commented Aug 14, 2018

Those warnings are extremely annoying. Actual error messages hiding between pages of warnings are harder to find than Waldo. I don't think they should be enabled before someone actually fixes them. In many cases it's not desirable to fix them, for example in tests you routinely assign to an unused variable just to ensure that something is assignment-compatible.

@dwijnand
Copy link
Member

@som-snytt
Copy link
Contributor Author

som-snytt commented Aug 14, 2018

The implicit evidence warnings go away after restarr. There are just a few real warnings (besides imports). I don't mind submitting a reversion, but I think in longer term, with so much churn in the code base, linting is useful, and it should be tuned for -Xfatal-warnings eventually. I'll do a reversion PR for your consideration.

The implicitNotFound errors are put behind -Xlint in the future where they are not enabled.

Modulo typos: #7069

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