Dagger: Generated code has "Parameter is hiding a field" warnings all over

[EmilyBjoerk]

The below is the generated code for one of my components:

  public CoreModule_ProvideUpdateCheckerFactory(
      Provider<Settings> aSettingsProvider,
      Provider<UpdateChecker.UpdateCallback> aUpdateCallbackProvider,
      Provider<String> aVersionProvider) {
    assert aSettingsProvider != null;
    this.aSettingsProvider = aSettingsProvider;
    assert aUpdateCallbackProvider != null;
    this.aUpdateCallbackProvider = aUpdateCallbackProvider;
    assert aVersionProvider != null;
    this.aVersionProvider = aVersionProvider;
  }

This occurs for just about all generated code.

This code causes a warning for each of the parameters to the tune of:

The parameter SettingsProvider is hiding a field from type CoreModule_ProvideUpdateCheckerFactory

This is caused by the formal parameter having the same name as the field in the class. The code works correctly thanks to qualification using this.. Could the code generation be change to set a prefix or suffix or something on the formal arguments to avoid this warning?

Warning for variable shadowing is default in Eclipse AFAICT.

Dagger version 2.10, Java 8.

[gk5885]

Oof. That's a pretty lame warning coming from Eclipse. That style of naming a constructor parameter to be the same as the field being assigned is extremely common and well-established (e.g. throughout Guava). I don't think that we're going to intentionally diverge from our typical coding style (even in the generated code) any time soon.

If it truly is the default setting, I would file a bug against Eclipse to get that changed.

[EmilyBjoerk]

The use of variable shadowing has mixed opinions, even on stackoverflow and related sites. I come from C++ where variable shadowing is a warning by default in both clang and GCC for example. Personally I believe that variable shadowing is bad practice as it can lead to subtle bugs if you're not careful. And we all know about the human factor.

Yes, the use of variable shadowing may be common in some Java code, but it's not universal nor required. We're talking about generated code that gets compiled in user projects, it should compile cleanly even with the strictest warning levels. Otherwise dagger is unnecessarily predicting project settings for the user which is bad style.

(Just to clarify, I don't care about the coding style in the generated code. I just want it to compile cleanly, even if it means adding @SuppressWarnings("hiding") on every class).

[JakeWharton]

That argument works equally well in reverse: "Otherwise your project
settings are enforcing the format and style of all generated code which is
bad style."

On Tue, Apr 18, 2017 at 3:40 AM Emily Björk [e-mail]
wrote:

The use of variable shadowing has mixed opinions, even on stackoverflow
and related sites. I come from C++ where variable shadowing is a warning by
default in both clang and GCC for example. Personally I believe that
variable shadowing is bad practice as it can lead to subtle bugs if you're
not careful. And we all know about the human factor.

Yes, the use of variable shadowing may be common in some Java code, but
it's not universal nor required. We're talking about generated code
that gets compiled in user projects, it should compile cleanly even
with the strictest warning levels. Otherwise dagger is unnecessarily
predicting project settings for the user which is bad style.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/google/dagger/issues/700#issuecomment-294715715, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEEfsiWLYxjwbUdidEOQ-GPwyZxmM3ks5rxGjdgaJpZM4M_Yiu
.

[EmilyBjoerk]

@JakeWharton I realise there might have been a confusion in my choice of wording, by "bad style" what I really mean is "bad practice" nothing to do with code style or formatting. I couldn't care less about coding style or formatting of the generated code, _as long as it compiles cleanly_.

As for your argument: How is it bad practice to generate code that compiles cleanly on strict warning settings?

[gk5885]

We hold the generated code to exactly that standard, but only for javac. Eclipse adds a whole bunch of other non-standard checks that change from release to release. We can't possibly track every new check added by every IDE or static analysis tool, so we drew the line at the standard compiler. I don't think that's going to change any time soon, especially given that this is a style check and not a correctness issue.

[EmilyBjoerk]

@gk5885 That's fair enough. All I want is a clean compile log and to have warnings for variable shadowing enabled for my code. Can we come up with a solution we both could be happy with?

Maybe something like an attribute to the @Component annotation that causes a class level @SuppressWarnings to be added? Like so: @Component(suppressWarnings="xyz") would add @SuppressWarnings("xyz") to the generated class definition. It wouldn't mess with your style, would be backwards compatible and would allow users to ignore any warnings from generated code that may clash with their project settings, and you wouldn't need to target anything but javac.

[tbroyer]

FWIW, Immutables generates a @SuppressWarnings("all"). It's understood by (some) IDEs, but not javac. Though honestly I think the right approach is to have the tooling be able to ignore warnings in @Generated code (like Error Prone does).

BTW, naming is hard enough to not add stupid rules like that. What would've you liked Dagger to emit? Prefix fields with m or f? Prefix or suffix the field or parameter name with a _?

[gk5885]

I think your best bet is to just deal with this on the eclipse end. See this Stack Overflow question and this bug. Apparently Eclipse runs afoul of quite a few code generators…

[EmilyBjoerk]

@gk5885 Thank you for your help, that resolves my problem. I looked everywhere except in the build path settings for a per folder setting...