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

Enforce var jdk11 applied #1609

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Enforce var jdk11 applied #1609

wants to merge 5 commits into from

Conversation

blacelle
Copy link
Contributor

@blacelle blacelle commented Mar 7, 2023 •
edited
Loading

This mirrors #1578 , and in addition it applied the mutator to the whole repository through:

mvn io.github.solven-eu.cleanthat:cleanthat-maven-plugin:apply -Dcleanthat.mutators=RSPEC-6212
./gradlew spotlessApply

(I relies on cleanthat mvn plugin not to have to drop the ratchetFrom parameter, and other steps, to force a full-process by Spotless).

Then, you can review the result of this rule.

Typically, some var may be missing due to lack of context (as each File is processed on a per-file basis)

@blacelle
Copy link
Contributor Author

blacelle commented Mar 7, 2023 

> Task :lib-extra:javadoc FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':lib-extra:javadoc'.
> Javadoc generation failed. Generated Javadoc options file (useful for troubleshooting): '/home/runner/work/spotless/spotless/lib-extra/build/tmp/javadoc/javadoc.options'

Would :lib-extra:javadoc not accept JDK11+ source code?

nedtwigg
nedtwigg reviewed Mar 7, 2023
View reviewed changes
gradle/spotless.gradle
@@ -17,6 +17,10 @@ spotless {
java {
ratchetFrom 'origin/main'
bumpThisNumberIfACustomStepChanges(1)
// `setMutators` will discard default mutators
// https://sonarsource.atlassian.net/browse/RSPEC-6212
cleanthat().version("2.9").sourceCompatibility(project.sourceCompatibility.toString()).clearMutators()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the sourceCompatibility could be set here?

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JavaExtension.java

Lines 376 to 378 in 6789a1d

if (isLicenseHeaderStep(step)) {
return step.filterByFile(LicenseHeaderStep.unsupportedJvmFilesFilter());
} else {

Something like if (isCleanThatStep(step)) && sourceCompatibility == null) { ...

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 did not suceed doing so in the initial iteration. having a new look with your suggestion, I'm still stuck as I can not find sourceCompatiblity in Project: https://github.com/gradle/gradle/blob/v7.6.1/subprojects/core-api/src/main/java/org/gradle/api/Project.java, there is no sourceCompatiblity.

Yet, project.sourceCompatibility is OK in gradle files. What am I missing here?

Copy link
Member

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/72040924

@nedtwigg
Copy link
Member

nedtwigg commented Mar 7, 2023

Wow, this is great, thanks! Re: javadoc, yes our javadoc generation is currently stuck at 8, I fixed it over here

d6c5ac2

but I haven't had time to get this merged yet. This PR will create a ton of merge conflicts, so I want to delay merging until the lint stuff has been merged. I've been swamped by just day-to-day maintenance recently, haven't had time to take on the structural issues.

thc202
thc202 reviewed Mar 7, 2023
View reviewed changes
...lipse-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/osgi/ResourceAccessor.java
File jarOrDirectory = new File(objUri.getPath());
var jarOrDirectory = new File(objUri.getPath());
Copy link
Contributor

@thc202 thc202 Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why wasn't the URI above replaced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first sight, it would be a limitation around type resolution, due to file being processed one a per-file basis. Let me have a deeper look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UnsolvedSymbolException{context='null', name='BundleException', cause='null'}. One may argue this information is not relevant in this purpose. However, here it is: JavaParser, the source -> AST used to manipulate Java-Code failed resolving the type of getBundleUri(clazz) as given method refers to a type unknown in the context of given source file.

Similar technical challenge around #1379 (comment).

For now, Cleanthat focuses on linting on a per-file basis (for similar reason Spotless processes (for now) each file individually).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation!

Copy link
Member

Choose a reason for hiding this comment

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

I would think that any assignment could be a var and we don't need to know anything about its type. The only reason not to var is if it's a declaration without an assignment. Am I wrong?

Copy link
Contributor Author

@blacelle blacelle Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You can find the unitTests for this mutator in LocalVariableTypeInferenceCases. There is multiple cases with @UnmodifiedMethod, expressing switching to a var would be invalid.

Even List<?> i = new ArrayList<String>(); should not be switched to var as you may later assign a CopyOnWriteArrayList while var would have turned semantically from List to ArrayList (and you can't assign a CopyOnWriteArrayList into an ArrayList).

We may then suppose final List<?> i = new ArrayList<String>(); could be turned to var, however I may be missing another edge-case, which would lead in Cleanthat producing invalid code.

@blacelle blacelle force-pushed the EnforceVar_jdk11_applied branch from 920c320 to 2fdba8f Compare March 30, 2023 05:02
@blacelle blacelle marked this pull request as ready for review March 30, 2023 05:02
@blacelle
Copy link
Contributor Author

@nedtwigg This has been resynced with main. Ready-for-merging

@nedtwigg
Copy link
Member

nedtwigg commented Apr 5, 2023

I want to delay merging until the lint stuff has been merged.

Blocked on

@nedtwigg nedtwigg added the pr-archive PRs which are still valid but have gotten stuck for some reason label Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thc202 thc202 thc202 left review comments

@nedtwigg nedtwigg nedtwigg left review comments

Assignees
No one assigned
Labels
pr-archive PRs which are still valid but have gotten stuck for some reason
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blacelle @nedtwigg @thc202