Skip to content

Fix nullability issue in queuechannel #3012

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.

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

Merged

Conversation

augur
Copy link
Contributor

@augur augur commented Jul 29, 2019

As per@Nullable annotation description,

Methods override should repeat parent {@code @nullable} annotations unless they behave differently.

QueueChannel inherits from AbstractPollableChannel, which has annotation @Nullable on doReceive, but QueueChannel lacked it. It presents a problem in some cases, see discussion here:
https://discuss.kotlinlang.org/t/override-java-method-return-type-nullability/13633

@garyrussell
Copy link
Contributor

I am not sure I understand - seems to be a bug in the compiler.

Are you saying the @Nullable is not inherited by the subclass?

I thought you only had to add @Nullable on the subclass if the super class has @NonNull and the subclass overrides the nullability.

@augur
Copy link
Contributor Author

augur commented Jul 29, 2019

I thought you only had to add @Nullable on the subclass if the super class has @NonNull and the subclass overrides the nullability.

Me too, but then I read description of this annotation: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/lang/Nullable.html

Methods override should repeat parent @nullable annotations unless they behave differently.

Thus, if I understand correct, even if super class has this annotation, sub class also must have it, otherwise it would have no effect.

@garyrussell
Copy link
Contributor

Hmmm - this works fine for me...

package com.example.demo;

import org.springframework.lang.Nullable;

public class Foo {

    @Nullable
    public Object getOne() {
        return "one";
    }

}

package com.example.demo;

public class Bar extends Foo {

    @Override
    public Object getOne() {
        return null;
    }

}
package kot

import com.example.demo.Bar

class Baz: Bar() {

    override fun getOne(): Any? {
        return "two"
    }

}

package kot

fun main() {
    val bar: Baz = Baz()
    val one: Any? = bar.one
    println(one)
}

What's the difference between this and your case?

@augur
Copy link
Contributor Author

augur commented Jul 29, 2019

Have you tested with -Xjsr305=strict compiler flag? This is default flag in Spring Initializr generated projects (spring-io/initializr#591)
More info about flag here: https://kotlinlang.org/docs/reference/java-interop.html

@garyrussell
Copy link
Contributor

Interesting, with that I get the error when subclassing QueueChannel but there are no complaints about my Foo/Bar/Baz friends.

I'll go ahead and merge this, but I fear there are many other places in the framework that will need to be addressed 😦

@garyrussell garyrussell merged commit ef1d7be into spring-projects:master Jul 29, 2019
garyrussell pushed a commit that referenced this pull request Jul 29, 2019
* Added inherited @nullable to QueueChannel.doReceive

* Minor refactoring
@garyrussell
Copy link
Contributor

Cherry-picked as e5fadc3 to 5.1.x

@artembilan
Copy link
Member

See @Inherited JavaDocs:

* <p>Note that this meta-annotation type has no effect if the annotated
 * type is used to annotate anything other than a class.  Note also
 * that this meta-annotation only causes annotations to be inherited
 * from superclasses; annotations on implemented interfaces have no
 * effect.

So, this @Nullable on super method has no effect on the overridden method.

@garyrussell
Copy link
Contributor

garyrussell commented Jul 29, 2019

No; I didn't mean @Inherited I meant that I expected that the Kotlin compiler would walk up the super classes to find the first annotation (like we do with Spring).

It does seem to silence Sonar, though.

@garyrussell
Copy link
Contributor

garyrussell commented Jul 29, 2019

Surprisingly, we only have 12 classes with abstract methods marked with @Nullable.

It won't catch them all but fixing those shouldn't be too bad; I'll start tomorrow.

Of course, some of them have many, many subclasses.

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