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

[MNG-7553] New clean API with immutable model #703

Merged
merged 107 commits into from
Oct 2, 2022

Conversation

@Tibor17
Copy link
Contributor

Tibor17 commented Apr 1, 2022

If we want to use immutable objects, we should use Java Records. We still do not have Valhalla ready.
The developers often do not understand Java Memory Model and there is no guarantee that they would not make a mistake when they update any immutable object. The developers usually would not understand why ConcurrentLinkedQueue is preferable over ArrayList and List.copyOf() is ideal https://docs.oracle.com/javase/10/docs/api/java/util/List.html#copyOf(java.util.Collection), and so we should use List.copyOf() which means that we should switch the project to Java 10 - bad luck! Nobody would accept Java 10, I guess.

The immutable object should be fetched with data via a builder in constructor.
MavenProject and MavenProjectBuilder composition

public record MavenProject(MavenProjectBuilder builder) {
 
    MavenProject {
        dependencies = List.copyOf(builder.getDependencies());
        ...
        ...
        ...
    }
}

@gnodet
Copy link
Contributor Author

gnodet commented Apr 1, 2022

If we want to use immutable objects, we should use Java Records. We still do not have Valhalla ready. The developers often do not understand Java Memory Model and there is no guarantee that they would not make a mistake when they update any immutable object. The developers usually would not understand why ConcurrentLinkedQueue is preferable over ArrayList and List.copyOf() is ideal https://docs.oracle.com/javase/10/docs/api/java/util/List.html#copyOf(java.util.Collection), and so we should use List.copyOf() which means that we should switch the project to Java 10 - bad luck! Nobody would accept Java 10, I guess.

The immutable object should be fetched with data via a builder in constructor. MavenProject and MavenProjectBuilder composition

public record MavenProject(MavenProjectBuilder builder) {
 
    MavenProject {
        dependencies = List.copyOf(builder.getDependencies());
        ...
        ...
        ...
    }
}

Have you had a look at the generated code or are those just general comments about java records / immutable objects ?

@Tibor17
Copy link
Contributor

Tibor17 commented Apr 1, 2022

Yes I saw the MDO.

@gnodet
Copy link
Contributor Author

gnodet commented Apr 1, 2022

Yes I saw the MDO.

So the objects from the model are immutable, they absolutely can not be modified.

So unless we raise the requirements to JDK 17, I don't see how we could actually use records / List.of(). That said, I'm not sure there's actually much difference at the end. Of course, using records would be less verbose, but given we don't have to maintain that code manually (because it's generated), I'm not sure it really matters. For the List.of(x) point, the model already uses immutable lists, though those use Collections.unmodifiableList(xxx) to be JDK 8 compatible.

@Tibor17
Copy link
Contributor

Tibor17 commented Apr 1, 2022

Don;t use Collections.unmodifiableList(xxx). It is vert old style - before year 2004 where the constructors where not thread safe, a typical findings with String and final fields - therefore Java changed impl for JMM of field semantics, used memory bariers at the end of constructor to make fields coherent with main mem, see JSR 133 https://download.oracle.com/otn-pub/jcp/memory_model-1.0-pfd-spec-oth-JSpec/memory_model-1_0-pfd-spec.pdf?AuthParam=1648805410_6b63e95379a8c30dbb165b2da54c9d0d

@gnodet
Copy link
Contributor Author

gnodet commented Apr 1, 2022

Don;t use Collections.unmodifiableList(xxx). It is vert old style - before year 2004 where the constructors where not thread safe, a typical findings with String and final fields - therefore Java changed impl for JMM of field semantics, used memory bariers at the end of constructor to make fields coherent with main mem, see JSR 133 https://download.oracle.com/otn-pub/jcp/memory_model-1.0-pfd-spec-oth-JSpec/memory_model-1_0-pfd-spec.pdf?AuthParam=1648805410_6b63e95379a8c30dbb165b2da54c9d0d

Sorry, I don't get the point of immutable collections vs memory semantics. I agree we should use List.of(xxx) if we were to raise to JDK 17, but I'm not sure there's a consensus there. What do you propose I use instead for immutable collections ? Also, in case you missed the point, I'm always using

list != null ? Collections.unmodifiableList( new ArrayList<>( list ) ) : Collections.emptyList()

in order to make sure the collections are actually safe and immutable.
I can re-implement a custom unmodifiable collection to avoid the double creation, but that's more an optimisation to me rather than an actual problem.

@gnodet
Copy link
Contributor Author

gnodet commented Sep 5, 2022

All tests are passing. I'd like to go on and merge this PR (and release an alpha version if possible). This would allow further work on other components and plugins (see the various PRs in this PR description) which are blocked because the API is not available in any public release or snapshot version.

@slawekjaranowski
Copy link
Member

👍 for merge and for release alpha

@cstamas
Copy link
Member

cstamas commented Sep 5, 2022

👍 as well, will take a deeper look tonight... but that should not block anything

@vlsi
Copy link

vlsi commented Sep 28, 2022

Consider using Nonnull by default, so the only place when you need to document nullability becomes @Nullable.

For instance, jspecify did not include @NonNull for a long time, and now they say @NonNull is rarely needed: https://github.com/jspecify/jspecify/blob/fa7b1d8654e5ec0696b5a8a8c49e275a2121aafd/src/main/java/org/jspecify/nullness/NonNull.java

* Introduce a Lookup service to retrieve beans from the PlexusContainer
* Improvements to the Node dependency graph
* Add access to the parent and remote repositories from the project
* Enhancements to better support Coordinate
* Add ProjectManager#setProperty
@gnodet gnodet changed the title New API with immutable model [MNG-7553] New clean API with immutable model Oct 2, 2022
@gnodet gnodet merged commit 2a9f393 into apache:master Oct 2, 2022
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.

10 participants