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.

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

Add <nullableAnnotationTargetElementType> and <nonnullAnnotationTargetElementType> to support generating ElementType.TYPE_USE annotations #10759

Open
jorsol opened this issue Oct 20, 2020 · 9 comments

Comments

@jorsol
Copy link

jorsol commented Oct 20, 2020

Expected behavior

Generated source code should be valid.

@Override
public org.jooq.@NonNull Catalog getCatalog() {
    return DefaultCatalog.DEFAULT_CATALOG;
}

Actual behavior

Illegally placed annotation: type annotations must directly precede the simple name of the type they are meant to affect (or the [] for arrays)

@Override
@NonNull
public org.jooq.Catalog getCatalog() {
    return DefaultCatalog.DEFAULT_CATALOG;
}

Steps to reproduce the problem

PostgreSQL database with a catalog schema has conflict with org.jooq.Catalog generating the nullable annotations in jOOQ 3.14.0, in 3.13.5 this annotation is not added to the method.

<nullableAnnotation>true</nullableAnnotation>
<nullableAnnotationType>org.eclipse.jdt.annotation.Nullable</nullableAnnotationType>
<nonnullAnnotation>true</nonnullAnnotation>
<nonnullAnnotationType>org.eclipse.jdt.annotation.NonNull</nonnullAnnotationType>

Versions

  • jOOQ: 3.14.0
  • Java: 15
  • Database (include vendor): PostgreSQL
  • OS: Linux
  • JDBC Driver (include name if inofficial driver): PostgreSQL 42.2.18
@lukaseder
Copy link
Member

Thanks a lot for your report.

The existing functionality assumes that the annotation supports ElementType.METHOD and ElementType.PARAMETER. Curiously, Eclipse didn't repurpose their existing annotations like Jetbrains did, but changed them incompatibly, it seems:
https://help.eclipse.org/2020-09/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Ftasks%2Ftask-using_null_type_annotations.htm&cp%3D1_3_9_1_3&anchor=compatibility

We've had this feature for a while now, always based on the existing assumptions. We could fix this by adding an additional set of configuration flags:

  • <nullableAnnotationTypeUse>true</nullableAnnotationTypeUse>
  • <nonnullAnnotationTypeUse>true</nonnullAnnotationTypeUse>

Until this is available, you can use an alternative set of annotations, e.g. the org.jetbrains.annotations ones.

@lukaseder lukaseder changed the title Illegally placed annotation: type annotations must directly precede the simple name of the type they are meant to affect Add <nullableAnnotationTypeUse> and <nonnullAnnotationTypeUse> to support generating ElementType.TYPE_USE annotations Oct 20, 2020
@lukaseder
Copy link
Member

You seem to see this as a regression based on the fact that we now generate this annotation on getCatalog(), but it was already generated on various other methods, instead of on the types. E.g. on Records or POJOs. Did you not get compilation errors before, on those classes, @jorsol ?

@jorsol
Copy link
Author

jorsol commented Oct 20, 2020

You seem to see this as a regression based on the fact that we now generate this annotation on getCatalog(), but it was already generated on various other methods, instead of on the types. E.g. on Records or POJOs. Did you not get compilation errors before, on those classes, @jorsol ?

I did not get compilation errors before, I'm assuming that the issue here is that collision having a schema called catalog then the generator create a public class Catalog extends SchemaImpl but to disambiguate the getCatalog() method it use the full name public org.jooq.Catalog getCatalog() exposing the Illegally placed annotation error on Eclipse. On another schema with different name it create a public Catalog getCatalog() method that have the @NonNull annotation without issues.

@lukaseder
Copy link
Member

Oh, I get it now. The correct syntax in that case would be org.jooq.@NonNull Catalog. Wow. I most definitely did not know this syntax...

It's not really a new issue, although indeed, it didn't manifest to you before. If there's a conflict with an import (e.g. when using CREATE TABLE INTEGER (I INT), which is then resolved with

<fullyQualifiedTypes>java.lang.Integer</fullyQualifiedTypes>

You'd get the same problem, even before 3.14. You can try it with the above <fullyQualifiedTypes/> configuration.

I don't think we can solve this automatically. An additional configuration will be needed to specify whether the annotation is ElementType.METHOD and ElementType.PARAMETER capable, or only ElementType.TYPE_USE

@lukaseder
Copy link
Member

I can think of three workarounds for you right now:

  • Use different annotations that are ElementType.METHOD and ElementType.PARAMETER capable
  • Turn off the generation of annotations
  • Use a generator strategy to rename the generated class for your catalog schema, e.g. to CatalogSchema or whatever

@jorsol
Copy link
Author

jorsol commented Oct 21, 2020

Ok, I will change to org.jetbrains.annotations as a workaround.

It would be great that the generator could automatically detect if it's only a ElementType.TYPE_USE annotation, but if an additional configuration will be needed it's also ok. Thanks for your input.

@lukaseder
Copy link
Member

It would be great that the generator could automatically detect if it's only a ElementType.TYPE_USE annotation, but if an additional configuration will be needed it's also ok. Thanks for your input.

Yes that sounds reasonable for a set of well-known annotations

@tbroyer
Copy link

tbroyer commented Apr 13, 2021

Fwiw, I'm using Checker Framework annotations (org.checkerframework.checker.nullness.qual.Nullable and org.checkerframework.checker.nullness.qual.NonNull) which are type annotations.
I also have arrays in my database.
jOOQ generates @NonNull String[] column instead of String @NonNull [] column in pojo constructors and setters, and @NonNull public String[] getColumn() instead of public String @NonNull [] getColumn() for pojo getters.

See https://checkerframework.org/manual/#faq-array-syntax-meaning for type annotations with arrays.

For now I just don't use nullable annotations, as my checker (Nullaway) is configured to treat @Generated classes as unannotated anyway (generated code doesn't pass jOOQ-checker's SQLDialectMatcher either).

@lukaseder
Copy link
Member

lukaseder commented Nov 1, 2021

A duplicate of this #12581. I think this has sufficient interest for inclusion in 3.16. I'll slightly adapt the specification here: #10759 (comment)

There are 3 types of behaviour for <nullableAnnotationTargetElementType/> and <nonnullAnnotationTargetElementType/>:

  • METHOD_AND_PARAMETER (the current behaviour)
  • TYPE_USE (the requested behaviour)
  • DEFAULT (which will try to find the proper behaviour automatically, based on the supplied type (package prefix). I'm assuming this will be TYPE_USE for most libraries)

@lukaseder lukaseder changed the title Add <nullableAnnotationTypeUse> and <nonnullAnnotationTypeUse> to support generating ElementType.TYPE_USE annotations Add <nullableAnnotationTargetElementType> and <nonnullAnnotationTargetElementType> to support generating ElementType.TYPE_USE annotations Nov 1, 2021
@lukaseder lukaseder removed this from To do in 3.16 Other improvements Jan 3, 2022
@lukaseder lukaseder removed this from the Version 3.18.0 milestone Jan 9, 2023
@lukaseder lukaseder added this to the Version 3.19.0 milestone Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants