Skip to content

Suppress unchecked warnings in generated builders#247

Closed
pkoenig10 wants to merge 1 commit into
palantir:developfrom
pkoenig10:unchecked
Closed

Suppress unchecked warnings in generated builders#247
pkoenig10 wants to merge 1 commit into
palantir:developfrom
pkoenig10:unchecked

Conversation

@pkoenig10
Copy link
Copy Markdown
Member

Before this PR

Conjure generated projects with covariant optional setters can not be compile with -Xlint:unchecked. This is because the improvements made in #9 requires casting optionals in certain cases.

After this PR

Conjure generated builders now suppress unchecked warnings. This seems reasonable since we control the code generation here.

We could attempt to be more targeted and only add this annotation to covariant optional setters but:

  • The builder generator code has relatively pure functions so passing this information back to the method level is not easy.
  • Adding the annotation to the builder level prevents regression if we require casting in other setters in the future.

@pkoenig10 pkoenig10 requested a review from a team as a code owner February 22, 2019 22:51
.build())
.addAnnotation(
AnnotationSpec.builder(SuppressWarnings.class)
.addMember("value", "$S", "unchecked")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At a minimum I think we need a comment here justifying why we add this to all builders... I just ran it locally and got:

> Task :conjure-java-core:compileIntegrationInputJava FAILED
/Users/dfox/conjure-java/conjure-java-core/src/integrationInput/java/com/palantir/product/CovariantOptionalExample.java:107: warning: [unchecked] unchecked cast
            this.item = (Optional<Object>) Preconditions.checkNotNull(item, "item cannot be null");
                                                                     ^
  required: Optional<Object>
  found:    Optional<CAP#1>
  where CAP#1 is a fresh type-variable:
    CAP#1 extends Object from capture of ?
error: warnings found and -Werror specified
1 error
1 warning

@iamdanfox
Copy link
Copy Markdown
Contributor

It feels pretty gross to be adding the @SuppressWarnings("unchecked") to the whole builder.

Could we have a stab at adding it to exactly the right methods?

        @JsonSetter("item")
+       @SuppressWarnings("unchecked")
        public Builder item(Optional<?> item) {
            this.item = (Optional<Object>) Preconditions.checkNotNull(item, "item cannot be null");
            return this;
        }

Also, if we want conjure-java to pass without warnings then I think this PR needs to also enable -Xlint:unchecked in the top level build.gradle to ensure we don't regress somewhere else!!

@carterkozak
Copy link
Copy Markdown
Contributor

Rather than adding suppressions to generated code, should we provide an Optional<Object> narrow(Optional<?>) utility function in the library, so we have a single suppression?

@iamdanfox
Copy link
Copy Markdown
Contributor

TBH I'd really like to keep conjure-lib as minimal as possible. If we could target these annotations accurately then most generated code wouldn't be affected, it would only be for people using any, which seems not too bad?

@iamdanfox
Copy link
Copy Markdown
Contributor

Closing out as I don't think it's really OK to suppress warnings on all builders - lmk if you want to have another stab @pkoenig10!

@iamdanfox iamdanfox closed this Mar 11, 2019
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.

3 participants