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

Clean up build warnings #35

Closed
wants to merge 12 commits into from
Closed

Clean up build warnings #35

wants to merge 12 commits into from

Conversation

TWiStErRob
Copy link
Contributor

... and prevent future ones.

Didn't want to go straight for adding -Werror, because it may clobber local hacking; but I think on CI the commands should include -Pstrict.compilation=true.

@TWiStErRob TWiStErRob mentioned this pull request Feb 8, 2019
Closed
@JakeWharton
Copy link
Owner

I've merged e62ad1b and 4f75ada. I'm still thinking about the others. I think bf2 might be obsoleted by a change I'm about to make to bindings and I need to think about 0c4 more. I'm all for more of the error-prone warnings but I'm a bit skeptical of the java ones. Maybe we can turn it on until they annoy me.

@TWiStErRob
Copy link
Contributor Author

0c4e911 is unlikely to produce anything other than unchecked, rawtypes and deprecation, which all would be visible in IDEA anyway as warnings, so quick to notice and usually very easy to fix. If "unlikely" is not good enough, it's always an option to flip it around from -Xlint:all -Xlint:-processing -Xlint:-classfile to -Xlint:unchecked -Xlint:rawtypes -Xlint:deprecation.

@TWiStErRob
Copy link
Contributor Author

Rebased and fixed all of the current warnings. gradlew clean check runs without warnings, except:

This unfixable warning from AGP:

> Task :integration-tests:android:processCodegenDebugResources
Changing the value for a property with a final value has been deprecated. This will fail with an error in Gradle 6.0.

and this Errorprone warning which is fixed in #156

> Task :reflect:compileJava
P:\projects\contrib\github-dagger2-reflect\reflect\src\main\java\dagger\reflect\Scope.java:82: warning: [ObjectToString] Linker is final and does not override Object.toString, so converting it to a string will print its identity (e.g. `Linker@ 4488aabb`) instead of useful information
.
                + linker); // TODO nice error message with scope chain
                  ^
    (see https://errorprone.info/bugpattern/ObjectToString)
1 warning

@TWiStErRob TWiStErRob changed the title Clean up generics warnings Clean up build warnings Aug 3, 2019
JakeWharton
JakeWharton reviewed Aug 5, 2019
View reviewed changes
Copy link
Owner

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

Mostly looks good. Just some nits.

integration-tests/upstream/build.gradle Show resolved Hide resolved
reflect/src/main/java/dagger/reflect/UnlinkedMapOfLazyBinding.java Outdated Show resolved Hide resolved
reflect/src/main/java/dagger/reflect/UnlinkedMapOfValueBinding.java Outdated Show resolved Hide resolved
reflect/src/main/java/dagger/reflect/UnlinkedSetBinding.java Outdated Show resolved Hide resolved
reflect/src/main/java/dagger/reflect/ReflectiveJustInTimeLookupFactory.java Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
@TWiStErRob
Swap annotation and visibility to match other code
576b616
@TWiStErRob
Fix lint warnings in integration test
68e4b95
@TWiStErRob
Fix Android Java annotation processor warning in integration test
472480b 
Fixes :integration-tests:android:javaPreCompileCodegenDebugUnitTest
"Annotation processors must be explicitly declared now.
The following dependencies on the compile classpath are found to contain annotation processor.
Please add them to the testAnnotationProcessor configuration.
See https://developer.android.com/r/tools/annotation-processor-error-message.html for more details."
@TWiStErRob
Specify Java compiler encoding explicitly to prevent using system def…
6da8209 
…ault.

Fixes :integration-tests:upstream:compileTestJava
"Cycles.java:35: error: unmappable character for encoding Cp1252"
@TWiStErRob
Fix rawtypes warning in reflect-lint
69d8b9e
@TWiStErRob
Fix rawtypes warnings in UnlinkedJustInTimeBinding
b795254
@TWiStErRob
Fix unchecked warnings in UnlinkedSetBinding by suppressing and expla…
6327f67 
…ining
@TWiStErRob
Fix unchecked warnings in UnlinkedMapOfProviderBinding by using <?> w…
5f9528b 
…hich is what the link() returns

The two objects in Map<Object, Provider<Object>> would be K and V anyway (as seen in LinkedMapOfValueBinding).
Changed covariant return type to match interface declaration and all other implementations of this interface.
@TWiStErRob
Fix unchecked warnings in UnlinkedMapOfValueBinding by suppression.
5bb0019 
When Scope constructs these objects it gives it the right key.
@TWiStErRob
Extract generic method to isolate type-safe code
8580905
@TWiStErRob
Extract generic method to isolate type-safe code
1b3687a
@TWiStErRob
Enable javac lint
1efaa52
@TWiStErRob
Copy link
Contributor Author

Double-force-pushed to resolve the conflict (deleted file in another PR) and see the code review changes:
https://github.com/JakeWharton/dagger-reflect/compare/a524045a02c38ec895911264f989ef7d7ea9e3fa..1efaa5293e48123358781e6067cebf83574c3e91

JakeWharton
JakeWharton reviewed Aug 6, 2019
View reviewed changes
reflect/src/main/java/dagger/reflect/UnlinkedMapOfProviderBinding.java
@@ -13,11 +13,10 @@
}

@Override
public LinkedBinding<Map<Object, Provider<Object>>> link(Linker linker, Scope scope) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is unfortunate. Not that I expect anyone to ever see these types, and they're erased anyway, but it at least forces a bit of safety into the method. Doing LinkedBinding<Map<?, Provider<?>>> doesn't work?

JakeWharton
JakeWharton reviewed Aug 6, 2019
View reviewed changes
build.gradle
'-Xlint:-processing',
// hide warning: MethodParameters attribute introduced in version 52.0 class files is ignored in version 51.0 class files
'-Xlint:-classfile',
// hide warning: auxiliary class C in full/path/to/C.java should not be accessed from outside its own source file
Copy link
Owner

Choose a reason for hiding this comment

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

What triggers all of these? I'm less interested in the description since that can be Googled but more interested in what causes them and why it's safe to ignore. Similar to an @Ignore for a test. The 'processing' one has a good justification but not these last two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All 3 are the warning messages, not explanation. I'll research and explain them in the context of this project.

@JakeWharton
Copy link
Owner

I've merged and cherry-picked about 7 or 8 of the commits in this PR.

@TWiStErRob
Copy link
Contributor Author

I've merged and cherry-picked about 7 or 8 of the commits in this PR.

ok, thanks; will rebase and continue to discuss the rest

@TWiStErRob
Copy link
Contributor Author

Only two relevant commits remain:

Closing this until the project becomes active again.

@TWiStErRob TWiStErRob closed this Feb 4, 2023
@TWiStErRob TWiStErRob deleted the generics_warnings branch February 4, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakeWharton JakeWharton JakeWharton left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TWiStErRob @JakeWharton