[MDSAL-244] @SuppressWarnings("all") on all generated code Created: 13/Apr/17  Updated: 08/Jun/18  Resolved: 08/Jun/18

Status: Resolved
Project: mdsal
Component/s: None
Affects Version/s: None
Fix Version/s: Fluorine

Type: Improvement
Reporter: Michael Vorburger Assignee: Robert Varga
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

Operating System: All
Platform: All

Issue Links:
Relates
relates to MDSAL-2 Naming conflict appears when key of l... Resolved

 Description   

All bindings code generated should have @SuppressWarnings("all").

This would make it much easier / viable to enable more stringent automated code quality checks e.g. in Eclipse without getting a lot of warnings on /target/generated-sources/mdsal-binding & Co.

Today for example you get a lot of warnings like:

  • "The method getModuleInfo() of type $YangModelBindingProvider should be tagged with @Override since it actually overrides a superinterface method"; and others related to @Override, like:
    • "The method getKey() of type NodeStatus should be tagged with @Override since it actually overrides a superinterface method"
    • "The method build() of type NodeStatusBuilder should be tagged with @Override since it actually overrides a superinterface method"
    • The method getImplementedInterface() of type NodeStatusBuilder.NodeStatusImpl should be tagged with @Override since it actually overrides a superinterface method
  • "The parameter augmentation is hiding a field from type {Some}

    Builder"

  • "The expression of type int is boxed into Integer"
  • "The import org.opendaylight.yang.gen.v1.urn.opendaylight.daexim.rev160921.DataStore.Enumeration is never used"
  • The parameter augmentation is hiding a field from type NodeStatusBuilder

etc. etc.

While it would be theoretically possible to fix up each of these one by one, this would likely be a fair amount of work, but with little benefit - none of those are "bugs" - the code generators knows what its known; hopefully, contrary to much of our hand written code.

Adding @SuppressWarnings("all") on all generated code probably also doesn't hurt (i.e. can only help) with build performance, if tools like javac, Eclipse' compiler and perhaps Find Bugs and Checkstyle honour and skip so annotated types in their analysis (I don't know if they do).

An alternative approach to putting @SuppressWarnings everywhere, specific to Eclipse only (and thus inferior already), would have been to mark target/generated-sources/mdsal-binding & Co. as "Ignore optional compile problems" in menu Project > Properties > Java Build Path > Source. I've explored this, but found that it is difficult to get M2E to do this automatically. (If you are brave enough to click around hundreds of projects yourself, then at least since https://bugs.eclipse.org/bugs/show_bug.cgi?id=388541 M2E no longer resets them; but we should not have to manually fiddle with such options in the first place - this should "just work" out of the box.)

I'll raise a Gerrit with a starting point proposal for this. As I'm not familiar with the finer points of the code generation in mdsal, I expect that initial proposed change will likely require some tuning by someone experienced in the mdsal team - my hope is that by kicking this off, someone will be motivated to help me finish it?

 Comments   
Comment by Michael Vorburger [ 13/Apr/17 ]

https://git.opendaylight.org/gerrit/#/c/54965/

Comment by Michael Vorburger [ 18/Apr/17 ]

> * "The expression of type int is boxed into Integer"

forget about this, that warning should not have been enabled

Comment by Martin Ciglan [ 19/Apr/17 ]

https://git.opendaylight.org/gerrit/#/c/54982/ - this one too

Comment by Michael Vorburger [ 09/May/17 ]

Re-assigning issue to Robert based on discussion in Kernel projects call today:

Robert is de-facto -2'ing https://git.opendaylight.org/gerrit/#/c/54965/, because he prefers to instead fix all code generators to not have warnings. Here's what's left as of today:

This can be seen using https://github.com/vorburger/opendaylight-eclipse-setup e.g. on daexim/model/target-ide/generated-sources/mdsal-binding; but like wise in other projects too.

  • The parameter augmentation is hiding a field from type CancelExportOutputBuilder; that's from the menu Window > Preferences > Java > Compiler > Errors/Warnings Name shadowing and conflicts set to level "Warning" for all 3 of:

a) Field declaration hides another field or variable
b) Local variable declaration hides another field or variable
c) Type parameter hides another type

  • Javadoc: Description expected after this reference DataStore.java line 62; that's from the menu Window > Preferences > Java > Compiler > Javadoc: [X] Process Javadoc comments, Malformed Javadoc comments: Warning as configured by opendaylight-eclipse-setup

/**

  • @param valueArg
  • @return corresponding Enumeration item
    */
    public static Enumeration forValue(int valueArg) { return VALUE_MAP.get(valueArg); }
  • The import org.opendaylight.yang.gen.v1.urn.opendaylight.daexim.rev160921.DataStore.Enumeration is never used: DataStore.java /org.opendaylight.daexim.daexim-model/target-ide/generated-sources/mdsal-binding/org/opendaylight/yang/gen/v1/urn/opendaylight/daexim/rev160921 line 3
  • The method getKey() of type ExcludedModules should be tagged with @Override since it actually overrides a superinterface method: ExcludedModules.java org.opendaylight.daexim.daexim-model/target-ide/generated-sources/mdsal-binding/org/opendaylight/yang/gen/v1/urn/opendaylight/daexim/rev160921/exclusions
Comment by Robert Varga [ 09/May/17 ]

So we are down to

  • getKey(), which is an API design mistake in v1, and therefore requires some special-casing in the templates around method generation (because it is metadata encoded as a user property)
  • unneeded import, which should boil down to using importedName()
  • shadowed variable name, which should be a simple matter of choosing a new name for the argument
  • and a missing argument description, which we should handle by not emitting it at all of the field does not a 'description' in the model.
Comment by Robert Varga [ 11/May/17 ]

https://git.opendaylight.org/gerrit/56833
https://git.opendaylight.org/gerrit/56834

After which we are down to two issues:

  • getKey() override
  • unused import, which actually is an import of an inner type, hence we need a bit of context to understand that we are in the same compilation unit.
Comment by Robert Varga [ 11/May/17 ]

https://git.opendaylight.org/gerrit/56840 fixes the import

Comment by Michael Vorburger [ 16/May/17 ]

> While it would be theoretically possible to fix up each of
> these one by one, this would likely be a fair amount of work

I've re-tested for this issue today, by running "mvn -Pide clean generate-sources" and seen the number of warnings on generated code e.g. in project genius drop drastically from ca. 2100 to now TBD. Here are the ones left (in genius; it's possible of course other projects have more still), all fall into 2 cases:

1. The method getKey() of type InterfaceMonitorEntry should be tagged with @Override since it actually overrides a superinterface method InterfaceMonitorEntry.java in e.g. org.opendaylight.genius.alivenessmonitor-api/target-ide/generated-sources/mdsal-binding/org/opendaylight/yang/gen/v1/urn/opendaylight/genius/alivenessmonitor/rev160411/_interface/monitor/map, line 59 ... indeed getKey() is on org.opendaylight.yangtools.yang.binding.Identifiable, but there is no @Override.

2. The field Mpls.QNAME is hiding a field from type InterfaceType Mpls.java in org.opendaylight.genius.interfacemanager-api/target-ide/generated-sources/mdsal-binding/org/opendaylight/yang/gen/v1/urn/opendaylight/genius/interfacemanager/rev160406, line 21 ... this is the public static final QName QNAME on a type (e.g. org.opendaylight.yang.gen.v1.urn.opendaylight.genius.interfacemanager.rev160406.Mpls) which extends another (e.g. org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.interfaces.rev140508.InterfaceType)

Comment by Robert Varga [ 24/Apr/18 ]

MDSAL-2 will take care of the first case, we cannot do anything about the second – we need a public class constant.

Comment by Robert Varga [ 08/Jun/18 ]

Warnings which have been identified are fixed.

Generated at Wed Feb 07 20:09:12 UTC 2024 using Jira 8.20.10#820010-sha1:ace47f9899e9ee25d7157d59aa17ab06aee30d3d.