Make optional setter covariant by JacekLach · Pull Request #9 · palantir/conjure-java

JacekLach · 2018-05-30T14:59:32Z

Before this PR

// imagine this comes from a function and you don't know if it's present or absent
Optional<String> specificValue = Optional.of("foo");
RequestArgs args = RequestArgs.builder()
  // optionalArgument is `optional<any>` because different requests take different types
  .optionalArgument(specificValue)
  .build();
args.getOptionalArgument() // returns Optional.of(Optional.of("foo"))

After this PR

Optional<String> specificValue = Optional.of("foo");
RequestArgs args = RequestArgs.builder()
  // optionalArgument is `optional<any>` because different requests take different types
  .optionalArgument(specificValue)
  .build();
args.getOptionalArgument() // returns Optional.of("foo")

JacekLach · 2018-05-30T15:25:27Z

Oh, I need to handle primitives specially here. Will fix later

markelliot · 2018-05-31T06:43:59Z

                            ByteBuffer.class)
                    .addStatement("this.$1N.rewind()", spec.name)
                    .build();
+        } else if (type.accept(TypeVisitor.IS_OPTIONAL)) {


Why is this change required?

You can't assign a covariant type to an invariant type, like Optional<? extends Object> bar = getBar(); Optional<Object> foo = bar;. You need to either explicitly recast and suppress compiler warnings, or transform the generic type with Optional.<Object>of(bar.get())

(actually, I checked and it looks like Optional<Object> foo = (Optional<Object>) bar; works without java warnings so maybe that's better)
(oh, actually, it does produce a warning but it seems to be suppressed in generated code by default)

I see. And you didn't want to widen the type everywhere?

We could widen the type for optionals everywhere, but that feels a bigger / more scary change to me. If maintainers think that's a better approach, I'm happy to follow suit.

Intuitively I feel like it might require changes in many places; if ConjureOptional.getItem() starts returning Optional<? extends FOO> then a lot of code that currently expects Optional<FOO> will break; and I don't see an advantage to getting a wildcard type when reading

I don't think there's much value in returning Optional<? extends FOO>, means you now have to propagate ? everywhere, and you can't assign it to a variable of type Optional<FOO> without a cast.
The value is there only when it's a parameter, since the compiler knows that FOO or e.g. BAR <: FOO both satisfy ? extends FOO.

markelliot · 2018-05-31T12:25:23Z

+            CodeBlock nullCheckedValue = Expressions.requireNonNull(
+                    spec.name, enriched.fieldName().get() + " cannot be null");
+            return CodeBlock.builder()
+                    .addStatement("this.$1N = $2L.flatMap($3T::<$4T>::of)",


just filling in the syntax to make it clearer:

foo.flatMap(Optional::<Object>::of)

doesn't seem valid to me/locally it does not compile for me.

there needs to be some testing around both the code gen and the logic of the codegen in this PR

Yeah, the extra :: was a typo that I already had fixed locally but forgot to push. I added a minor test but currently the infra for adding new cases is missing; will come back to it after #10 is fixed

uschi2000 · 2018-05-31T12:47:09Z

what's a concrete use case for this? typically, our conjure objects have concrete types and there's no class hierarchy.

JacekLach · 2018-05-31T13:08:55Z

@uschi2000 the biggest benefit is for Optional<any>, which sees some usage.

With current bindings, you get this gotcha:

 // imagine this comes from a function and you don't know if it's present or absent
Optional<String> specificValue = Optional.of("foo");
RequestArgs args = RequestArgs.builder()
  // optionalArgument is `optional<any>` because different requests take different types
  .optionalArgument(specificValue)
  .build();
args.getOptionalArgument() // returns Optional.of(Optional.of("foo"))

With wildcard builder you get Optional.of("foo") in the last example, which I think you more often expect. Of course it means if you actually want a directly nested optional in an untyped field you need to put in some more boilerplate; but that seems a very rare occurrence.

uschi2000 · 2018-05-31T13:42:26Z

+        String value = "foo";
+        Optional<String> maybeValue = Optional.of(value);
+
+        // FIXME: this doesn't compile right now because changes to example-types.yml aren't picked up


there's a test here that re-generates the code when you comment out a if (REGEN) - ish statement

I believe that regenerates the java code based on the .json definitions, but there's nothing that regenerates the .json definitions right now, see #10

JacekLach · 2018-06-01T17:22:23Z

Green!

markelliot · 2018-06-01T18:33:08Z

Given the use-case, I wonder if we only want to be covariant for any to limit abuse.

JacekLach · 2018-06-01T18:50:59Z

I think it's also possibly handy for things like collections (being able to put an Optional<SortedSet> into a optional<set<string>>), but it is only in the case of any that without this change you transparently use the wrong overload, instead of getting a compilation error. So I'm OK with just doing this for any at first, and extending if real use-case shows up.

Even though most of the items we take are final, it's better to be covariant on everything than not be covariant on anything - in particular covariance for collections of Objects makes for a more natural api, where `Optional<String> foo = Optional.of("foo"); return AnyOptional.of(foo);` results in `Optional.of("foo")` rather than `Optional.of(Optional.of("foo"))`; We could in theory track if the class being contained is final (a conjure object / a primitive), but it doesn't seem worth the effort; taking `Optional<? extends String>` seems OK even if nothing can extend string.

JacekLach · 2018-08-28T16:57:12Z

Another related annoyance is the static builders for collections:

    public static CovariantListExample of(List<Object> items) {
        return builder().items(items).build();
    }

but at a quick glance it doesn't seem trivial to widen the parameters there (the BeanBuilderGenerator and BeanGenerator seem to operate differently)

dansanduleac · 2018-08-29T11:29:09Z

        @JsonSetter("item")
        public Builder item(Optional<String> item) {
-            this.item = Objects.requireNonNull(item, "item cannot be null");
+            this.item = (Optional<String>) Objects.requireNonNull(item, "item cannot be null");


this cast doesn't seem necessary

yep, missed out on fixing tests after 5435159

530b826 should address this and fix tests

iamdanfox · 2018-08-29T16:54:18Z


        @JsonSetter("doubleItems")
-        public Builder doubleItems(Iterable<Double> doubleItems) {
+        public Builder doubleItems(Iterable<? extends Double> doubleItems) {


As a rule of thumb, we've tried to make conjure-java generate code that is as close as possible to what a human might write. Can we avoid the ? extends for known classes like String, Double, Integer etc?

iamdanfox · 2018-08-29T16:56:06Z


        @JsonSetter("errors")
-        public Builder errors(Iterable<ErrorDefinition> errors) {
+        public Builder errors(Iterable<? extends ErrorDefinition> errors) {


I'm also not a fan of this. When a builder accepts a conjure type, we know it is final so there's no point adding ? extends

iamdanfox · 2018-09-03T16:11:09Z

@JacekLach Dan and I had a couple of CR comments above - what do you think?

JacekLach · 2018-09-03T16:20:02Z

I agree with the sentiment behind them, just didn't have time to look into how to make it happen. The answer is probably to inspect if the type is final somehow.

JacekLach · 2018-09-03T16:53:39Z

@iamdanfox I think this does it.

I'll let the .of constructors be for now even though they make me sad, since at least you can now use the builder when you need to :P

Non-any primitives, conjure references, and optionals are always final, and so there's no point in widening functions that take iterables of those.

iamdanfox · 2018-09-04T16:40:06Z

        }

-        public Builder addAllArgs(Iterable<ArgumentDefinition> args) {
+        public Builder addAllArgs(Iterable<? extends ArgumentDefinition> args) {


I think these shouldn't have changed?

iamdanfox

I think this makes sense to me as it prevents the nasty footgun you encountered (putting Optional.of(x) into a builder method, resulting in an unwanted double-nested optional).

The code necessary to achieve this does seem pretty hard to read, but I think the outcome is probably worth it.

* Add ConjureGeneratorParameters as a map builder * don't allow nulls * ConjureGeneratorParametersRenderer * stop throwing groovy exceptions * make it static * Rewrite ConjureExtension to use ConjureGeneratorParameters for typescript and java * Allow false to fall through as --flag=false * rename to generatorParameters * enforce camelCase key names * CompileConjureJavaTask - capture out/err, log args at info * Use rewritten feature flag retrofitCompletableFutures * Rename ConjureGeneratorParameters -> GeneratorOptions * GeneratorOptions copy constructor * Fixup conjure typescript compilation according to Felipe's changes * Bump conjure-java to 0.2.4 * generatorOptionsSupplier -> options * lolz * go away errorprone, tis not a service * disable Slf4jLogsafeArgs cos we are not a service / consumed by a service * no dist * conjure-typescript 0.5.0 * unnnecessary * one line * log info in typescript too * log if override is same as defaultˆ * Move GeneratorOptions, ConjureExtension to a gradle-conjure-api * Move GeneratorOptions, ConjureExtension to a gradle-conjure-api * Use additionalArgs * Move default parameter handling t oRenderGeneratorOptions * oops * fix bug again, add tests * getTypescript / getJava * And get rid of copy constructor now that we don't need it anymore * simplify * only support defaults * no conjure core here * don't depend on conjure-core, just guava

markelliot reviewed May 31, 2018

View reviewed changes

uschi2000 reviewed May 31, 2018

View reviewed changes

JacekLach added 2 commits August 28, 2018 17:52

Add a test exercising covariant builders

43012c4

Only widen optionals of Object exactly

5435159

dansanduleac reviewed Aug 29, 2018

View reviewed changes

Simplify optional widening; fix tests

530b826

iamdanfox reviewed Aug 29, 2018

View reviewed changes

Don't widen iterables of final types

0f3a63c

Non-any primitives, conjure references, and optionals are always final, and so there's no point in widening functions that take iterables of those.

iamdanfox reviewed Sep 4, 2018

View reviewed changes

JacekLach added 2 commits September 4, 2018 17:48

Undo accidental changes to conjure/spec definitions

b53c8d2

Widen optionals the same way as iterables

7b1528a

iamdanfox approved these changes Sep 4, 2018

View reviewed changes

iamdanfox merged commit 7a00cb7 into palantir:develop Sep 5, 2018

iamdanfox deleted the jl/optional-covariance branch September 5, 2018 15:51

iamdanfox mentioned this pull request Oct 15, 2018

Optional field builder should be covariant #8

Closed

pkoenig10 mentioned this pull request Feb 22, 2019

Suppress unchecked warnings in generated builders #247

Closed

iamdanfox mentioned this pull request May 22, 2019

Having an optional<any> generates an unchecked cast #390

Open

JacekLach mentioned this pull request Sep 9, 2019

Generated java for optional<set<...>> types can fail to compile #536

Closed

Conversation

JacekLach commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before this PR

After this PR

Uh oh!

JacekLach commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JacekLach May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uschi2000 commented May 31, 2018

Uh oh!

JacekLach commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JacekLach commented Jun 1, 2018

Uh oh!

markelliot commented Jun 1, 2018

Uh oh!

JacekLach commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JacekLach commented Aug 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamdanfox commented Sep 3, 2018

Uh oh!

JacekLach commented Sep 3, 2018

Uh oh!

JacekLach commented Sep 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamdanfox left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JacekLach commented May 30, 2018 •

edited

Loading

JacekLach May 31, 2018 •

edited

Loading

JacekLach commented May 31, 2018 •

edited

Loading

JacekLach commented Jun 1, 2018 •

edited

Loading