Fwd: Re: RFR: JEP 359-Records: compiler code
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Fri Nov 1 12:56:31 UTC 2019
On 01/11/2019 12:06, Maurizio Cimadamore wrote:
> I really don't get why the message shows the erasure of the methods -
> you are calling:
>
> Errors.ConstructorWithSameErasureAsCanonical(other, canonical)
>
> That is, you are passing symbols - the symbols contain full type info
> - why is this being erased? This doesn't seem to happen e.g. in
> ordinary clash errors, e.g.
>
> InheritanceConflict3.java:14:10: compiler.err.name.clash.same.erasure:
> f(java.lang.Object), f(T)
>
> Is it possible, by any chance that we're generating the canonical
> symbol with an erased type?
Did some more investigation on this. The problem was trivial - and was
made clearer by compiling an example like this:
import java.util.List;
record Foo(List<String> ls) {
Foo(List<Integer> li) { }
}
This generates:
Test.java:4: error: signature clash: constructor Foo(List<Integer>) and
canonical constructor Foo(List<Integer>) have the same erasure
Foo(List<Integer> li) { }
^
1 error
Where it's clear that there's no erasure at play; the real issue is that
the underlying diagnostic is bogus:
compiler.err.constructor.with.same.erasure.as.canonical=\
signature clash: constructor {0} and canonical constructor
{0} have the same erasure
Note the double reference to {0}. If this is fixed as:
# 0: symbol, 1: symbol
compiler.err.constructor.with.same.erasure.as.canonical=\
signature clash: constructor {0} and canonical constructor {1} have
the same erasure
Then the error displayed is correct:
# 0: symbol, 1: symbol
compiler.err.constructor.with.same.erasure.as.canonical=\
signature clash: constructor {0} and canonical constructor {1} have
the same erasure
Also, while experimenting, I noted that compiling code like:
record Foo(String s) { }
Without the --enable-preview flag, causes a spurious parser error, with
no clue that I'm using a preview feature:
Test.java:1: error: class, interface, or enum expected
record Foo(String s) {
^
1 error
Is that expected? I believe the issue is with 'isRecordToken' which
always returns 'false' if preview features are disabled. I think
something should call checkSourceLevel(Feature.RECORDS), so that preview
errors for records will be triggered.
And, looking more, I see some issues with parsing inside blocks - for
instance, this program no longer compiles (w/o enable-preview):
class Foo {
void test() {
final record r = null;
}
}
class record {}
That's because when we're inside a method body, we always interpret
'final record' as a record declaration.
A better solution would be to always check (with lookahead) for "record
IDENT '('". That should be more robust.
In other words, I think that the parser code should be consolidated so
that you have a single routine that checks for the start of a record
declaration:
record IDENT '('
If the above production is matched (with lookahead), then you call
checkSourceLevel(Feature.RECORDS), so that the correct errors are generated.
The routine could be something like this:
boolean isRecordStart() {
if (token.kind == IDENTIFIER && token.name() == names.record &&
(peekToken(TokenKind.IDENTIFIER, TokenKind.LPAREN) ||
peekToken(TokenKind.IDENTIFIER, TokenKind.LT))) {
checkSourceLevel(Feature.RECORDS);
return true;
} else {
return false;
}
}
Then you remove isRecordToken, and start to use the above everywhere. I
tried with a crude patch (see below), and things seem to work much
better. Examples:
record foo(int x) {}
gives:
Test.java:1: error: records are a preview feature and are disabled by
default.
record foo(int x) {}
^
(use --enable-preview to enable records)
1 error
And, this:
class Foo {
void test() {
final record r = null;
}
}
class record {}
Gives the following:
Test.java:3: warning: 'record' may become a restricted type name in a
future release and may be unusable for type declarations or as the
element type of an array
final record r = null;
^
Test.java:7: warning: 'record' may become a restricted type name in a
future release and may be unusable for type declarations or as the
element type of an array
class record {}
^
2 warnings
Which I think it's closer to what we want.
Patch below:
diff -r 1da4a8addd21
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.javaThu
Oct 31 23:59:56 2019 -0400
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.javaFri
Nov 01 12:53:31 2019 +0000
@@ -2558,7 +2558,7 @@
if (token.kind == INTERFACE ||
token.kind == CLASS ||
token.kind == ENUM ||
- (token.kind == IDENTIFIER && token.name() ==
names.record) ) {
+ isRecordStart()) {
return
List.of(classOrRecordOrInterfaceOrEnumDeclaration(mods, dc));
} else {
JCExpression t = parseType(true);
@@ -2625,9 +2625,7 @@
//else intentional fall-through
}
}
- if (isRecordToken() &&
- (peekToken(TokenKind.IDENTIFIER, TokenKind.LPAREN) ||
- peekToken(TokenKind.IDENTIFIER, TokenKind.LT))) {
+ if (isRecordStart()) {
dc = token.comment(CommentStyle.JAVADOC);
return List.of(recordDeclaration(F.at(pos).Modifiers(0), dc));
} else {
@@ -3661,7 +3659,7 @@
protected JCStatement
classOrRecordOrInterfaceOrEnumDeclaration(JCModifiers mods, Comment dc) {
if (token.kind == CLASS) {
return classDeclaration(mods, dc);
- } if (isRecordToken()) {
+ } if (isRecordStart()) {
return recordDeclaration(mods, dc);
} else if (token.kind == INTERFACE) {
return interfaceDeclaration(mods, dc);
@@ -4022,7 +4020,7 @@
int pos = token.pos;
JCModifiers mods = modifiersOpt();
if (token.kind == CLASS ||
- isRecordToken() ||
+ isRecordStart() ||
token.kind == INTERFACE ||
token.kind == ENUM) {
return
List.of(classOrRecordOrInterfaceOrEnumDeclaration(mods, dc));
@@ -4117,9 +4115,16 @@
}
}
- boolean isRecordToken() {
- return allowRecords && token.kind == IDENTIFIER && token.name()
== names.record;
- }
+ boolean isRecordStart() {
+ if (token.kind == IDENTIFIER && token.name() == names.record &&
+ (peekToken(TokenKind.IDENTIFIER, TokenKind.LPAREN) ||
+ peekToken(TokenKind.IDENTIFIER, TokenKind.LT))) {
+ checkSourceLevel(Feature.RECORDS);
+ return true;
+ } else {
+ return false;
+ }
+}
/** MethodDeclaratorRest =
* FormalParameters BracketsOpt [THROWS TypeList] (
MethodBody | [DEFAULT AnnotationValue] ";")
Cheers
Maurizio
More information about the amber-dev
mailing list