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

8254275: [valhalla/jep390] Revise "value-based class" & apply to wrappers #222

Closed
wants to merge 11 commits into from

Conversation

dansmithcode
Copy link
Collaborator

@dansmithcode dansmithcode commented Oct 9, 2020

Polishing the specification of "value-based class" to align with requirements of inline classes, allow classes (like Integer) with deprecated constructors, and clarify expectations for clients.

Update cross-references from existing value-based classes and the primitive wrapper classes.

Full docs build: http://cr.openjdk.java.net/~dlsmith/8254275/8254275-20201124/api/index.html


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build (6/6 running) (2/2 running) (2/2 running) (2/2 running)

Issue

  • JDK-8254275: [valhalla/jep390] Revise "value-based class" & apply to wrappers

Reviewers

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/222/head:pull/222
$ git checkout pull/222

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 9, 2020

👋 Welcome back dlsmith! A progress list of the required criteria for merging this PR into jep390 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Oct 9, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 9, 2020

mlchung
mlchung approved these changes Oct 9, 2020
Copy link
Member

@mlchung mlchung left a comment

This looks fine to me.

@openjdk
Copy link

openjdk bot commented Oct 9, 2020

@dansmithcode This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8254275: [valhalla/jep390] Revise "value-based class" & apply to wrappers

Reviewed-by: mchung, rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the jep390 branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the jep390 branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 9, 2020
Copy link
Collaborator

@RogerRiggs RogerRiggs left a comment

The existing value-based classes do not adhere to the new statements.
These changes are re-writing the spec after the fact.
Existing classes do not have to be final and can extend anything they want.

@dansmithcode
Copy link
Collaborator Author

dansmithcode commented Oct 9, 2020

The existing value-based classes do not adhere to the new statements.
These changes are re-writing the spec after the fact.
Existing classes do not have to be final and can extend anything they want.

Yes, this is a deceptively-small-looking change to the spec for a large number of classes.

You'll have to clarify which classes you're concerned about, though. I just verified that all of the following classes are both final and extend an appropriate superclass:

KeyValueHolder, MapN, Map1, SetN, Set12, ListN, List12, Optional, OptionalDouble, OptionalInt, OptionalLong, OffsetTime, ZonedDateTime, Duration, Instant, LocalDateTime, LocalTime, YearMonth, Year, MonthDay, OffsetDateTime, ZoneRegion, ZoneOffset, MinguoDate, HijrahDate, JapaneseDate, ThaiBuddhistDate, ProcessHandleImpl, Runtime.Version

(I stopped before digging into the ConstantDesc and foreign classes...)

@RogerRiggs
Copy link
Collaborator

RogerRiggs commented Oct 13, 2020

I was most concerned about the primitive wrapper classes, that are not currently specified to be value based (currently do not have references to ValueBased.html or similar statements). The existing classes wrapper classes have some explicit identity requirements in the JLS.
Adding the @valuebased annotation, with a javadoc reference to the updated ValueBased.html would be a specification change.
For JEP 390, the intention is to enable warnings, not to change the specification.
So either the @valuebased annotation is weaker than a spec change or the link to ValueBased.html must be a future aspirational statement, not a link to a current specification requirement.

@mlchung
Copy link
Member

mlchung commented Oct 13, 2020

For JEP 390, the intention is to enable warnings, not to change the specification.
So either the @valuebased annotation is weaker than a spec change or the link to ValueBased.html must be a future aspirational statement, not a link to a current specification requirement.

I guess you may be concerning about the potential compatibility risks by this proposed spec change. Of course JEP 390 can make spec change if discussed and agreed. There is no behavioral change to the primitive wrapper classes except the warnings are emitted if synchronized on these wrapper objects.

@RogerRiggs can you clarify more what you are concerned about?

@RogerRiggs
Copy link
Collaborator

RogerRiggs commented Oct 13, 2020

Yes, the compatibility concern is foremost. We don't intend to change the behavior (yet) so the exact language used to describe what is being changed is important. A simple assertion that Integer is ValueBased breaks compatibility.
The JLS requirement for reference equality for small values is at odds with ValueBased.

@dansmithcode dansmithcode changed the title 8254275: Development to revise ValueBased.html for consistency with inline class migration 8254275: Development to revise "value-based class" & apply to wrappers Oct 13, 2020
@dansmithcode
Copy link
Collaborator Author

dansmithcode commented Oct 13, 2020

To clarify my thinking on the identity/cacheing behavior of wrappers:

  • Integer should claim to be a value-based class. I've made that part of this issue, and will include it in another iteration of the code.

  • ValueBased.html needs to allow for factory methods that guarantee consistent identity for at least some results. It's okay to relax the definition of "value-based class" in this way—it doesn't change the contract of other value-based classes' factories, just gives them the option to do cacheing if they want.

  • When Integer becomes a primitive class, the consistent identity guarantee of Integer.valueOf will continue to be true, and will extend to the full domain of Integers.

@RogerRiggs
Copy link
Collaborator

RogerRiggs commented Oct 13, 2020

Byte, Short, Integer, and Long all have caches for small values.

@dansmithcode
Copy link
Collaborator Author

dansmithcode commented Oct 13, 2020

Yes, understood. I was using Integer as an example, but the above should apply to all of those classes.

@dansmithcode
Copy link
Collaborator Author

dansmithcode commented Oct 13, 2020

New revision: I updated the definition, especially with respect to ==, and applied the boilerplate to the wrapper classes. I realized the changed definition requires changing the boilerplate everywhere, so I've done so, fixing up all the references to ValueBased.html.

Please be hyper-critical of the boilerplate text, as this appears in lots of places—it's worth getting it just right.

@dansmithcode
Copy link
Collaborator Author

dansmithcode commented Oct 13, 2020

I added a link to a docs build in the summary.

Copy link
Collaborator

@RogerRiggs RogerRiggs left a comment

In the text referring to ValueBase.html in many classes:

Is the "," misplaced. Should it read:
"not use instances for synchronization or unpredictable behavior may occur."

If the comment in each class was a simple reference to ValueBased, it would be easier to maintain and there would be less duplication between lots of classes and the explanation in ValueBased.html.

<li>have implementations of <code>equals</code>,
<li>declare only final instance fields (though these may contain references
to mutable objects);</li>
<li>declare implementations of <code>equals</code>,
<code>hashCode</code>, and <code>toString</code> which are computed
solely from the instance's state and not from its identity or the state
of any other object or variable;</li>
Copy link
Collaborator

@RogerRiggs RogerRiggs Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quibble on pre-existing text:

If the instance state is a reference to another object, can its hashCode be included in the hashCode?
The " not... from the state of any object" would seem to prohibit that.

Copy link
Collaborator Author

@dansmithcode dansmithcode Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps "not from its identity or the state of any other object or variable that is not part of the instance's state"?

Copy link
Collaborator Author

@dansmithcode dansmithcode Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, the "any other object or variable" clause is too strong. What about a toString based on System.lineSeparator() or something like that? Seems like it's trying too hard to prohibit some kind of workaround to mutable state. But I think "computed solely" already communicates that pretty clearly.

How about just: "which are computed solely from the values of the class's instance fields (and properties of the objects they reference), not from the instance's identity"

<li>perform no synchronization on an instance's intrinsic lock;</li>
<li>do not have (or have deprecated any) accessible constructors;</li>
<li>may support instance creation through factory methods that do <em>not</em>
promise a unique identity for each invocation—in particular, each factory
Copy link
Collaborator

@RogerRiggs RogerRiggs Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the factory method are in a value-based classes, they are not allowed to promise a unique identity.
So that statement is always true in the value-based context.

Use "," instead of "-" or start a new sentence.

Copy link
Collaborator Author

@dansmithcode dansmithcode Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Not allowed to promise a unique identity" based on what? Which other bullet in this list? There's the assertion about being "freely substitutable", but that doesn't mean a factory method can't promise to create a new instance on every invocation.

serialization, or any other identity-sensitive mechanism.</p>
<p>Synchronization on instances of value-based classes is strongly discouraged,
because the programmer cannot usually guarantee unique ownership of the
associated lock.</p>
Copy link
Collaborator

@RogerRiggs RogerRiggs Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the phrase:
because the programmer cannot usually guarantee unique ownership of the associated lock.
adds anything and is obscure. Until the semantics of object change, there is no change in the synchronization behavior.
For Valhalla value-based instances, there is explicitly no (implicitly) associated lock, so synchronization is illegal.

Copy link
Collaborator Author

@dansmithcode dansmithcode Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that if you can't prove you have a unique identity (per the factory method clause), then you can't guarantee that you have exclusive control over the monitor, so synchronization is dangerous. A variant of the text discouraging synchronization was there before any design discussions about primitive object semantics.

I added "usually" because of the possibility that someone can prove that a field value they provide is unique (according to 'equals') and thus can prove that the value-based class instance is also unique. But when I think about actual examples of value-based classes, that seems quite unlikely, and it would probably be clearer to remove the "usually".

@jodastephen
Copy link
Contributor

jodastephen commented Oct 16, 2020

Can I suggest some additions?

  • java.time.temporal.ValueRange - meets the criteria AFAICT

  • java.time.format.DecimalStyle - meets the criteria AFAICT despite implementation internal cache of instances

  • java.time.Clock subclasses - abstract base class equals/hashCode looks hostile, but JDK impls like SystemClock meet the criteria AFAICT

  • java.time.chrono.AbstractChronology subclases - all five subclasses meet the criteria AFAICT

I don't think java.time.temporal.WeekFields meets the criteria as the factory promises "same instance", although it is conceptually value-based.

java.util.Currency is conceptually value-based, but equals/hashCode isn't suitable at present - I think this should be worked on, as currency is a key value-based concept.

@dansmithcode
Copy link
Collaborator Author

dansmithcode commented Oct 16, 2020

In the text referring to ValueBase.html in many classes:

Is the "," misplaced. Should it read:
"not use instances for synchronization or unpredictable behavior may occur."

Grammatically, it's a compound sentence. Two statements: 1) programmers should do some stuff; 2) otherwise, unpredictable behavior. The comma helps to communicate where the "parentheses" go: (1a && 1b) || 2.

But I think you may be concerned that the "unpredictable behavior" only applies to the synchronization part of (1), not the "interchangeable" part? It's my intent that it applies to both. If you try to distinguish between instances with ==, or do synchronization, behavior will be unpredictable (because it's possible things that were != yesterday will be == today, per the rules about factories).

If the comment in each class was a simple reference to ValueBased, it would be easier to maintain and there would be less duplication between lots of classes and the explanation in ValueBased.html.

Yes. This approach of scattering boilerplate throughout the API is not optimized for maintainability. But that's okay, because change will not be frequent, and meanwhile it's optimized for getting readers' attention instead. I would worry that if we stripped out all the boilerplate and just had a link, most readers wouldn't follow the link or appreciate what it meant.

are equal according to <code>equals()</code> produces no visible change in
the behavior of the class's methods;</li>
<li>perform no synchronization using an instance's monitor;</li>
<li>do not have (or have deprecated any) accessible constructors;</li>
Copy link
Collaborator

@RogerRiggs RogerRiggs Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having an accessible constructor should not be prohibited.
Constructors do not imply or contradict the other constraints.
And when we get to Valhalla, primitive classes are allowed to have accessible constructors.
At least for the time being, the wrapper classes DO have accessible constructors; if the constraint is retained, then Integer, etc cannot be value-based.

Copy link
Collaborator Author

@dansmithcode dansmithcode Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea behind all the instance creation restrictions is this: clients of value-based classes should never be promised a unique, private identity associated with any instances they are given. By designing the API in this way, we discourage clients from inappropriate dependencies on identity, and pave the way for the classes to more smoothly migrate to be primitive classes someday.

Yes, primitive classes can have public constructors. But if a migrated primitive class has a public constructor, its clients will face binary and behavioral incompatibilities when the migration happens. If clients are using factories that don't promise unique identities, and aren't doing risky synchronization on objects they don't uniquely control, they will encounter neither of those incompatibilities.

The "or have deprecated any" rule allows for classes like the wrapper classes that do have accessible constructors, but have used deprecation to discourage clients from using them. Deprecation is a strong enough signal that when clients face future incompatibilities, they will have been sufficiently warned.

<li>perform no synchronization using an instance's monitor;</li>
<li>do not have (or have deprecated any) accessible constructors;</li>
<li>do not provide any other instance creation mechanism that promises
a unique identity on each method call&mdash;in particular, any factory
Copy link
Collaborator

@RogerRiggs RogerRiggs Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mdash came out as "â" in the javadoc.

Copy link
Collaborator Author

@dansmithcode dansmithcode Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That's annoying. In what context? Worked okay when I did a make docs...

Are character entities supposed to work? Or is the coding convention to stick strictly to ASCII?

method must allow for the possibility that if two independently-produced
instances are equal according to <code>equals()</code>, they may also be
equal according to <code>==</code>;</li>
<li>are final;</li>
Copy link
Collaborator

@RogerRiggs RogerRiggs Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to be able to mark an abstract class as @valuebased to document that it is part of an intended ValueBased class hierarchy. Perhaps add "or be abstract".

Copy link
Collaborator Author

@dansmithcode dansmithcode Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this has been handled until now is to say "all implementing classes should be value-based" in the abstract class's (or factory method's) javadoc. In terms of prose, that seems to get the job done, without needing to complicate the definition of value-based class.

For the annotation, yes the intent is that it will be applied both to value-based classes and to abstract classes/interfaces that require all implementing classes to be value-based.

serialization, or any other identity-sensitive mechanism.</p>
<p>Synchronization on instances of value-based classes is strongly discouraged,
because the programmer cannot guarantee exclusive ownership of the
associated monitor.</p>
Copy link
Collaborator

@RogerRiggs RogerRiggs Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicates the requirements in the bullet list.
The bullet list seems too long and the statements are not all orthogonal, making it a bit less clear.
Suggestion:

  • Combine bullet for class is final, with the first bullet requiring final fields.
  • combing the bullets on substitutability and equality, defining substtitutability in terms of the methods.

If the paragraph above is kept, move it to before the bullet list, using the bullet list at the details that explain the more general understanding of value-based.

The last sentence is still problematic. For current usage, there is a monitor.
For Valhalla, there is no monitor and it will be a runtime error. I don't think exclusive ownership comes into it.
And it duplicates an item in the bullet list.

Copy link
Collaborator Author

@dansmithcode dansmithcode Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revise to clarify that the bulleted list is about properties of the class declaration, while the subsequent sentences are constraints on/advice to clients. (Is there a better way to talk about clients than "a program should not..."? "Programmer", "client", "user" all kind of work, but I don't love any of them...) With that distinction in mind, I don't think the client stuff works very well until we first define what we mean by "value-based class".

I did remove what seemed to me like a redundant bullet from the original list (see my earlier comment). With what's left, each one is trying to say something distinct, although maybe some rephrasing in certain places could help.

The final class and field restrictions were combined before, but I split them because they're two very different constraints—one about extension, the other about immutability. (It's not ideal that Java uses the same keyword for both properties.) Actually, though, it works pretty well to combine the final class restriction with the superclass restriction, leaving one bullet all about subclassing. I'll do that.

The bullets that talk about equality are saying 1) the class needs an appropriate equals/hashCode/toString; 2) the class's instance methods don't have distinct behaviors for instances that are equals; 3) the class's factories do not promise distinct identities for instances that are equals. Those are three distinct things to say; I'm not seeing a clear way to combine them.

Synchronization: forget primitive classes. This is advice to current clients of value-based classes. And the advice is: don't synchronize, because you can't be sure someone else isn't doing the same on the same object. This is advice that is relevant to current clients without asking them to imagine a future in which the Object & monitor model has changed. Yet the implication is the same: don't do it.

@dansmithcode
Copy link
Collaborator Author

dansmithcode commented Oct 16, 2020

Pushed some updates; here's an updated docs build: http://cr.openjdk.java.net/~dlsmith/8254275/8254275-20201016/api/index.html

@dansmithcode
Copy link
Collaborator Author

dansmithcode commented Oct 19, 2020

Can I suggest some additions?

Thanks for the suggestions, I've initiated a discussion about them. For this issue & JEP, we'll restrict ourselves to classes that already claim to be value-based (plus the wrappers). But it would be worthwhile to separately explore applying the term to additional Java SE classes. It can be handled as an RFE for each of the relevant APIs.

@dansmithcode dansmithcode changed the title 8254275: Development to revise "value-based class" & apply to wrappers 8254275: [valhalla/jep390] Revise "value-based class" & apply to wrappers Oct 28, 2020
according to <code>equals()</code> in any computation or method
invocation should produce no visible change in behavior.
</li>
solely from the values of the class's instance fields (and properties of
Copy link
Collaborator

@RogerRiggs RogerRiggs Nov 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the word "properties" be avoided,m here and above? It it a loaded term. Perhaps attributes?

Copy link
Collaborator Author

@dansmithcode dansmithcode Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"members" would be the idiomatic Java word, I suppose

visible change in the behavior of the class's methods;</li>
<li>the class performs no synchronization using an instance's monitor;</li>
<li>the class does not declare (or has deprecated any) accessible constructors;</li>
<li>the class does not provide any other instance creation mechanism that promises
Copy link
Collaborator

@RogerRiggs RogerRiggs Nov 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "other" in "any other" is unnecessary.
The class does not provide any instance creation mechanism that promises a unique identity.

I don't know that the second part means in practice. What does 'allow for the possibility', apply to?
It seems apply to the factory method. Is there an example where the factory method needs to take some particular action or make some condition true? If two instances are == then it is more likely that other methods would be interested in that, not the factory method.

Copy link
Collaborator Author

@dansmithcode dansmithcode Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I can drop "other".

The intent is that there's a restriction on a factory method's contract. I can make that explicit: "any factory method's contract must allow for the possibility..."

<li>the class is final, and extends either <code>Object</code> or a hierarchy of
abstract classes that declare no instance fields or instance initializers
and whose constructors are empty.</li>
<li>
</ul>

Copy link
Collaborator

@RogerRiggs RogerRiggs Nov 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps as an intro to the following points to make it clear these are about what a program should and should not do.
Should use equals(); should use explicit synchronization using lock objects or instances that are not value-based classes, etc.
Programs should not attempt to distinguish the identities of value based class instances, otherwise the result may be unpredictable.

Copy link
Collaborator Author

@dansmithcode dansmithcode Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally following. This is a revision to the "When two instances" paragraph? Can you propose an alternative phrasing for the entire paragraph?

Copy link
Collaborator

@RogerRiggs RogerRiggs Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two paragraphs are fine, they express the negative, not what a developer should do.

mlchung
mlchung approved these changes Nov 2, 2020
<li>have implementations of <code>equals</code>,
<li>the class declares only final instance fields (though these may contain references
to mutable objects);</li>
<li>the class declares implementations of <code>equals</code>,
Copy link
Collaborator

@RogerRiggs RogerRiggs Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does saying the class "declares" the methods apply to Records and other classes that have provided or generated methods for equals, hashcode, and toString? Must the class explicitly declare the methods to meet the criteria?

Perhaps "the implementations of equals, hashCode, and tostring use only the instance's state...".

Copy link
Collaborator Author

@dansmithcode dansmithcode Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated fields and methods are "implicitly declared". But I suppose acceptable implementations could be inherited from a superclass, if the superclass isn't Object. I'll fix.

@openjdk
Copy link

openjdk bot commented Nov 24, 2020

@dansmithcode this pull request can not be integrated into jep390 due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8254275
git fetch https://git.openjdk.java.net/valhalla jep390
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge jep390"
git push

@openjdk openjdk bot added merge-conflict and removed ready labels Nov 24, 2020
@openjdk openjdk bot added ready and removed merge-conflict labels Nov 24, 2020
Copy link
Collaborator

@RogerRiggs RogerRiggs left a comment

Looks good, thanks for the updates.

@dansmithcode
Copy link
Collaborator Author

dansmithcode commented Nov 24, 2020

/integrate

@openjdk openjdk bot closed this Nov 24, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 24, 2020
@openjdk
Copy link

openjdk bot commented Nov 24, 2020

@dansmithcode Pushed as commit cdaf144.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants