[jira] [Comment Edited] (CALCITE-4520) In ImmutableBeans, nullable properties become not-nullable in project that does not use checkerframework

["Vladimir Sitnikov (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-4520?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17294816#comment-17294816 ] 

Vladimir Sitnikov edited comment on CALCITE-4520 at 3/3/21, 8:48 PM:
---------------------------------------------------------------------

1) This is not a bug for Calcite. I do not see how it blocks the release
2) I would argue ImmuableBeans is Calcite-internal API.
3) Calcite exposes (it should) the checker framework annotations, so downstream projects would see checker framework {{@Nullable}} annotations. If you happen to use Calcite's class, then you need to adapt the code when upgrading Calcite.
4) As far as I know, the next big thing would be https://github.com/jspecify/jspecify , however, it is not ready yet. I believe, Checker Framework's nullable annotations are the best for now.

{quote}I think that nullable properties should be indicated by an annotation that we control {quote}
There's no point in controlling the nullability annotations. The domain is non-trivial, and the checker framework has great amount of tooling and documentation. Let's reuse it. Then we would migrate to jspecify which would again be supported by the major tools.

---

If you absolutely want to control the nullness annotation, I would suggest that you could add your own Nullable annotation in Morel, and you could adjust ImmuableBeans to accept "any package" annotation like {{className.endsWith(".Nullable")}}.


---

{{required = false}} is a huge step backward. Whatever you do, please do not use {{required = false}} pattern. I believe in our case `nullable type` vs `non-nullable type` conveys the meaning very well, and {{nullable}} vs {{non-nullable}} types improves IDE and verification support. {{required = true/false}} has absolutely no tooling support when it comes to setters and getters.


was (Author: vladimirsitnikov):
1) This is not a bug for Calcite. I do not see how it blocks the release
2) I would argue ImmuableBeans is Calcite-internal API.
3) Calcite exposes (it should) the checker framework annotations, so downstream projects would see checker framework {{@Nullable}} annotations. If you happen to use Calcite's class, then you need to adapt the code when upgrading Calcite.
4) As far as I know, the next big thing would be https://github.com/jspecify/jspecify , however, it is not ready yet. I believe, Checker Framework's nullable annotations are the best for now.

{quote}I think that nullable properties should be indicated by an annotation that we control {quote}
There's no point in controlling the nullability annotations. The domain is non-trivial, and the checker framework has great amount of tooling and documentation. Let's reuse it. Then we would migrate to jspecify which would again be supported by the major tools.

---

If you absolutely want to control the nullness annotation, I would suggest that you could add your own Nullable annotation in Morel, and you could adjust ImmuableBeans to accept "any package" annotation like {{className.endsWith(".Nullable")}}.


---

{{required = false}} is a huge step backward. Whatever you do, please do not use {{required = false}} pattern. I believe in our case `nullable type` vs `non-nullable type` conveys the meaning very well, and {{nullable}} vs {{non-nullable}} types improves IDE and verification support.

> In ImmutableBeans, nullable properties become not-nullable in project that does not use checkerframework
> --------------------------------------------------------------------------------------------------------
>
>                 Key: CALCITE-4520
>                 URL: https://issues.apache.org/jira/browse/CALCITE-4520
>             Project: Calcite
>          Issue Type: Bug
>            Reporter: Julian Hyde
>            Priority: Major
>             Fix For: 1.27.0
>
>
> In ImmutableBeans, nullable properties become not-nullable in project that does not use checkerframework. In my Morel project, I get the following error:
> {noformat}
> java.lang.ExceptionInInitializerError
> 	at org.apache.calcite.prepare.CalcitePrepareImpl.<clinit>(CalcitePrepareImpl.java:154)
> 	at org.apache.calcite.tools.Frameworks.withPrepare(Frameworks.java:182)
> 	at org.apache.calcite.tools.RelBuilder.create(RelBuilder.java:225)
> 	at net.hydromatic.morel.foreign.Calcite.<init>(Calcite.java:42)
> 	at net.hydromatic.morel.foreign.Calcite$CalciteMap.<init>(Calcite.java:64)
> 	at net.hydromatic.morel.foreign.Calcite.withDataSets(Calcite.java:56)
> 	at net.hydromatic.morel.Ml.assertEvalSame(Ml.java:319)
> 	at net.hydromatic.morel.AlgebraTest.checkEqual(AlgebraTest.java:234)
> 	at net.hydromatic.morel.AlgebraTest.testNative(AlgebraTest.java:230)
> 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> 	at java.base/java.lang.reflect.Method.invoke(Method.java:567)
> 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
> 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
> 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
> 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
> 	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
> 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
> 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
> 	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
> 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
> 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
> 	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
> 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
> 	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
> 	at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
> 	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
> 	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
> 	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:230)
> 	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:58)
> Caused by: java.lang.IllegalArgumentException: property 'org.apache.calcite.adapter.enumerable.EnumerableLimitSortRule$Config#Description' is required and has no default value
> 	at org.apache.calcite.util.ImmutableBeans.lambda$makeDef$0(ImmutableBeans.java:168)
> 	at org.apache.calcite.util.ImmutableBeans$BeanImpl.invoke(ImmutableBeans.java:480)
> 	at com.sun.proxy.$Proxy15.description(Unknown Source)
> 	at org.apache.calcite.plan.RelRule.<init>(RelRule.java:115)
> 	at org.apache.calcite.adapter.enumerable.EnumerableLimitSortRule.<init>(EnumerableLimitSortRule.java:35)
> 	at org.apache.calcite.adapter.enumerable.EnumerableLimitSortRule$Config.toRule(EnumerableLimitSortRule.java:57)
> 	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:719)
> 	at org.apache.calcite.util.ImmutableBeans.lambda$makeDef$3(ImmutableBeans.java:299)
> 	at org.apache.calcite.util.ImmutableBeans$BeanImpl.invoke(ImmutableBeans.java:480)
> 	at com.sun.proxy.$Proxy15.toRule(Unknown Source)
> 	at org.apache.calcite.adapter.enumerable.EnumerableRules.<clinit>(EnumerableRules.java:90)
> 	... 31 more
> {noformat}
> The property {{EnumerableLimitSortRule.Config.Description}} is declared as nullable (because it has the annotation {{org.checkerframework.checker.nullness.qual.Nullable}}) but that annotation does not come through when the class is used in the Morel project, and therefore the property becomes non-nullable.
> This bug was introduced by the [~vlsi]'s change CALCITE-4284.
> We should not require client projects to use checkerframework, but if they don't, they will get this error when they reference nullable properties.
> I think that nullable properties should be indicated by an annotation that we control - go back to using {{ImmuableBeans.Property(required = false)}}.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)