RFR: JDK-8227046: compiler implementation for sealed classes, JDK-8227047: Javadoc for sealed types and JDK-8227044: javax.lang.model for sealed classes
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Tue May 26 14:48:48 UTC 2020
Looks good.
For the diagnostic, longer term it would be nice to generalize those:
# 0: token, 1: token
2168 compiler.err.expected2=\
2169 {0} or {1} expected
2170
2171 # 0: token, 1: token, 2: token
2172 compiler.err.expected3=\
2173 {0}, {1}, or {2} expected
2174
2175 # 0: token, 1: token, 2: token, 3: string
2176 compiler.err.expected4=\
2177 {0}, {1}, {2}, or {3} expected
2178
To work on more argument kinds (since you need the same thing).
I would at leas attempt to follow the same text as in the other
diagnostics - I don't see "one of:" being used anywhere else in
compiler.properties.
Maurizio
On 26/05/2020 05:14, Vicente Romero wrote:
> Hi Maurizio,
>
> Right good catch I forgot to add the MONKEY_AT to the list of expected
> tokens. I have fixed that. I have published another iteration at [1].
> Apart from the MONKEY_AT issue addressed at the parser this iteration
> also:
>
> - adds another error key to compiler.properties, this new key is
> oriented to show a more explicit error message when an interface with
> no `non-sealed` or `sealed` modifier extends a sealed interface. A
> class in this position can also be `final` but this is not possible
> for interfaces.
> - changes to PrintingProcessor and to
> test/langtools/tools/javac/processing/model/element/TestSealed.java
> suggested by Joe in this thread
> - adds a new subtest at SealedCompilationTests testing the sealed and
> non-sealed modifiers being followed by different modifiers or annotations
> - modified a subtest also at SealedCompilationTests, testPrinting,
> that was failing on Windows
>
> Thanks,
> Vicente
>
> [1] http://cr.openjdk.java.net/~vromero/8227046/webrev.03.delta/
>
> On 5/25/20 6:37 AM, Maurizio Cimadamore wrote:
>> Hi,
>> changes look good, but the parser changes do not convince me. Now
>> that the logic is more clearly defined, I see some issues e.g. there
>> seems to be a tight coupling by where "sealed" or "non-sealed" is,
>> and the fact that the following token must be e.g. "class". This is
>> unlike other modifiers which can in fact appear in any configuation.
>>
>> I believe that you can break the current logic by adding an extra
>> annotation between the "sealed" token and the "class" token e.g.
>>
>> sealed @foo class Bar { }
>>
>> But maybe the quick fix would be to add MONKEY_AT to the set of
>> expected tokens after sealed/non-sealed.
>>
>> Maurizio
>>
>>
>> On 22/05/2020 19:25, Vicente Romero wrote:
>>> Hi,
>>>
>>> Thanks Jan and Maurizio for the reviews. I have created another
>>> iteration that I hope addresses all of the comments. I recognize
>>> that dealing with `sealed`, `non-sealed` is the most complicated
>>> part as there is no guide right now from the spec. So I have tried
>>> to make them contextual keywords, for some informal definition of
>>> the concept, I think that with more success for the `sealed` case.
>>> So going in more detail this iteration includes:
>>>
>>> - ClassTree::getPermitsClause now returns `List.of()`
>>> - Types::findDescriptorInternal has been modified to fail on sealed
>>> interfaces
>>> - several error messages have been updated as suggested, on this
>>> front given that when a class list the same interface twice in the
>>> implements clause, the second occurrence is the one flagged, I did
>>> the same for repeated names in the permits clause
>>> - modifications in ClassReader and ClassWriter as suggested
>>> - JavacParser, the bulk of the changes are concentrated here, as
>>> mentioned above the goal has been to try to implement the contextual
>>> keyword thing and also give a nice error message in several corner
>>> case situations detected in the reviews
>>> - more tests were added
>>>
>>> Thanks,
>>> Vicente
>>>
>>> On 5/21/20 8:14 AM, Maurizio Cimadamore wrote:
>>>> Hi Vicente,
>>>> looks very good. Some comments below.
>>>>
>>>> * the parser logic is clever in its use of position to apply
>>>> context-dependent keyword detection; as Jan says, perhaps just
>>>> share the code so that the position checks are not repeated.
>>>>
>>>> * I found one very edge-case quirk in the context-dependent logic;
>>>> not sure how we wanna address:
>>>>
>>>> class Foo {
>>>> sealed m() {}
>>>> }
>>>>
>>>> This fails with:
>>>>
>>>> Error: invalid method declaration; return type required
>>>>
>>>> As javac parses non-sealed as a modifier, and then expects a type.
>>>> I think this is probably reasonable, but it's not as
>>>> context-dependent as it could be I guess.
>>>>
>>>> * This case:
>>>>
>>>> class Bar { }
>>>> sealed @interface Foo permits Bar
>>>>
>>>> Fails as expected, but only because Bar doesn't extends Foo. I
>>>> believe we'd like to ban sealed on annotations more eagerly. Same
>>>> for non-sealed. For enums and records (which are non-extensible)
>>>> the compiler does the right thing and tells me that I can't just
>>>> use sealed/non-sealed there.
>>>>
>>>> * The recovery logic in case preview features aren't enabled leaves
>>>> something to be desired. For instance, if I compile this w/o
>>>> --enable-preview:
>>>>
>>>> record Foo() {}
>>>>
>>>> I get a very sensible error:
>>>>
>>>> records are a preview feature and are disabled by default.
>>>> (use --enable-preview to enable records)
>>>>
>>>> However, if I compiler this w/o --enable-preview:
>>>>
>>>> sealed class Foo {}
>>>>
>>>> I get this:
>>>>
>>>> error: class, interface, or enum expected
>>>>
>>>> (no mention of preview features)
>>>>
>>>> It gets worse if I also specify a `permits`.
>>>>
>>>> * As Jan mentioned, type parameters on permitted types should be
>>>> banned, not silently cleared in TypeEnter
>>>>
>>>> * Overall the type enter logic seems robust - I've tried several
>>>> examples swapping superclass/subclass - using references to nested
>>>> classes in supertype declaration, and it all works. Well done.
>>>>
>>>> * The error for lambda expressions leaves to be desired:
>>>>
>>>> sealed interface Action {
>>>> void doAction();
>>>> }
>>>>
>>>> class Test {
>>>> Action a = () -> { };
>>>> }
>>>>
>>>> Foo.java:6: error: class is not allowed to extend sealed class: Action
>>>> Action a = () -> { };
>>>> ^
>>>>
>>>> I think a dedicated error here would be useful.
>>>>
>>>>
>>>> * the same check is not applied to method references:
>>>>
>>>>
>>>> class Test {
>>>>
>>>> Action a2 = Test::m; //no error
>>>>
>>>> static void m() { }
>>>> }
>>>>
>>>> More generally, if a functional interface cannot be sealed, I think
>>>> it would be better to inject the check in the functional interface
>>>> check (e.g. Types::findDescriptorInternal) so that you won't need
>>>> any extra code in Attr. This would also be more in spirit with the
>>>> spec, where the non-sealedness check is defined in 9.8, not in
>>>> section 15.
>>>>
>>>> * Pulling more on that string, the @FunctionalInterface annotation
>>>> can be placed on a sealed interface and no error is issued
>>>>
>>>> * On ClassWriter - isn't calling adjustFlags() enough? That will
>>>> truncate the long flags into an int - I think Flags.SEALED is
>>>> bigger than that?
>>>>
>>>>
>>>> // error messages
>>>>
>>>> * DuplicateTypesInPermits
>>>>
>>>> I suggest:
>>>>
>>>> test/langtools/tools/javac/diags/examples/DuplicateTypeInPermits.java:30:
>>>> error: invalid permits clause
>>>> sealed class Sealed permits Sub, Sub {}
>>>> ^
>>>> (repeated type: Sub)
>>>>
>>>> [this is consistent with the error we issues in other places - e.g.
>>>> when you implements same interface twice]
>>>>
>>>> * NonSealedWithNoSealedSuper
>>>>
>>>> test/langtools/tools/javac/diags/examples/NonSealedWithNoSealedSuper.java:31:
>>>> error: non-sealed modifier not allowed here
>>>> non-sealed class Sub extends C {}
>>>> ^
>>>> (class must have a sealed superclasses)
>>>>
>>>> I suggest to replace the details message with something like this:
>>>>
>>>> (class C does not have any sealed supertypes)
>>>>
>>>> [since I expect this message to be applicable also for
>>>> superinterfaces]
>>>>
>>>> * PermitsCantListDeclaringClass
>>>>
>>>> test/langtools/tools/javac/diags/examples/PermitsCantListDeclaringClass.java:30:
>>>> error: invalid permits clause
>>>> sealed class C permits C {}
>>>> ^
>>>> (must not include the declaring class: C)
>>>>
>>>> Here I recommend something like:
>>>>
>>>> (illegal self-reference in permits clause)
>>>>
>>>> * PermitsCantListSuperType
>>>>
>>>> test/langtools/tools/javac/diags/examples/PermitsCantListSuperType.java:32:
>>>> error: invalid permits clause
>>>> sealed class C implements I permits I {}
>>>> ^
>>>> (must not include a supertype: I)
>>>>
>>>> I suggest:
>>>>
>>>> (illegal reference to supertype I)
>>>>
>>>> * PermitsInNoSealedClass
>>>>
>>>> test/langtools/tools/javac/diags/examples/PermitsInNoSealedClass.java:30:
>>>> error: invalid permits clause
>>>> class C permits Sub {}
>>>> ^
>>>> (class must be sealed)
>>>>
>>>> This is good, but I noted that if you change the test to use an
>>>> interface, the message still says "class" - the kindname should be
>>>> used here.
>>>>
>>>> * SealedMustHaveSubtypes
>>>>
>>>> test/langtools/tools/javac/diags/examples/SealedMustHaveSubtypes.java:29:
>>>> error: sealed class must have subclasses
>>>> sealed class Sealed {}
>>>> ^
>>>>
>>>> I think this message reflects one of the main issues with the
>>>> general type vs. class dichotomy. A subclass, in JLS lingo is e.g.
>>>> `B` where `B extends A`. Interfaces do not play in the mix - they
>>>> are not considered subclasses. The word subtypes could be more
>>>> general - but again, it is a bit imprecise, since we're talking
>>>> about declarations here, not types. I'll defer this conundrum to
>>>> our spec gurus :-)
>>>>
>>>>
>>>> Cheers
>>>> Maurizio
>>>>
>>>>
>>>>
>>>> On 18/05/2020 23:42, Vicente Romero wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this patch for the compiler, javadoc and
>>>>> javax.lang.model support for the JEP 360 Sealed Classes (Preview).
>>>>> The changes are provided at [1], which implements the latest JLS
>>>>> for sealed types [2]. The patch also include the needed changes to
>>>>> javadoc and javax.lang.model to support sealed types. The CSR
>>>>> witht the changes in the javax.lang.model spec is at [3]. The
>>>>> sealed types JEP is accessible at [4]. There is an ongoing review
>>>>> for the VM and core-libs code of sealed types [5] and that code
>>>>> hasn't been included in this webrev,
>>>>>
>>>>> Thanks,
>>>>> Vicente
>>>>>
>>>>> [1] http://cr.openjdk.java.net/~vromero/8227046/webrev.00/
>>>>> [2]
>>>>> http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jls.html
>>>>>
>>>>> [3] https://bugs.openjdk.java.net/browse/JDK-8244367
>>>>> [4] https://openjdk.java.net/jeps/360
>>>>> [5]
>>>>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066440.html
>>>
>
More information about the core-libs-dev
mailing list