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

Can't override static builder for the builder-in-disguise pattern #1467

Open
nezda opened this issue Jun 6, 2023 · 3 comments
Open

Can't override static builder for the builder-in-disguise pattern #1467

nezda opened this issue Jun 6, 2023 · 3 comments

Comments

@nezda
Copy link

nezda commented Jun 6, 2023

Can't override static builder for the builder-in-disguise pattern (from #234 (comment) (and #432 (comment))) which is referenced in the docs

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project value-fixture: Compilation failure
[ERROR] /Users/nezda/code/immutables/value-fixture/target/generated-sources/annotations/org/immutables/fixture/modifiable/ImmutableBuilderInDisguise.java:[156,51] error: builder() in ImmutableBuilderInDisguise cannot hide builder() in BuilderInDisguise
[ERROR] return type Builder is not compatible with BuilderInDisguiseBuilder

Demonstrated in PR #1466

@elucash
Copy link
Member

elucash commented Jun 7, 2023

I may not understand it well, I'll reread tomorrow. But from the first impression.
One static method cannot hide another if return type differs (iirc) [given that same signature - no params]. In order to un-clash those signatures something have to be done.

  • Rename default builder method (@Style.builder = "anotherBuilder" | "new" )
  • Disable standard builder (@Value.Immutable(builder = false), might require @Style.allParameters
  • or smth like that...

@nezda
Copy link
Author

nezda commented Jun 7, 2023

You've got it exactly right @elucash . I tried adding @Value.Immutable(builder = false) but as you said, probably requires @Style.allParameters as it failed to compile with error below. If I'm interpreting the builder-in-disguise recipe correctly, this is sort of a bug / unanticipated? Not being able to use the name builder hobbles the disguise a little bit depending on the name you pick (for my actual use case I called it modifiableBuilder in the abstract class. Thinking about it more, renaming the immutable builder with something like @Style.builder = "immutableBuilder" seems better and works fine. Maybe the documentation for this feature should suggest this?

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project value-fixture: Compilation failure
[ERROR] /Users/nezda/code/immutables/value-fixture/src/org/immutables/fixture/modifiable/BuilderInDisguise.java:[19,25] error: Attribute 'name' is mandatory and should be a constructor @Value.Parameter when builder is disabled or there are other constructor parameters

@elucash
Copy link
Member

elucash commented Jun 8, 2023

Yes, I would say the clash is a natural and unanticipated consequence. I dunno, maybe adding some validation or documentation might help, but just not enough time/resources/motivation to polish these rarely used areas. These renamings, as they are, exist as a sharp tool. If I [ever] will rewrite immutables I would, probably, construct and validate generation model and validate it thoroughly (extensibility -> plugins -> need to detect / resolve conflicts). Any PRs for javadocs, or if we just copy paste some comments into builder-in-disguise ticket – always welcome

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

No branches or pull requests

2 participants