RFR: 8273408: java.lang.AssertionError: typeSig ERROR on generated class property of record [v2]

Guoxiong Li gli at openjdk.java.net
Thu Sep 16 12:41:54 UTC 2021


On Wed, 15 Sep 2021 14:35:09 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Treat source as comment mistakenly. Remove the comment symbol.
>
> I think that there are two test cases missing here. I think that you should investigate the interaction of your patch with something like:
> 
> 
> record R(unknownType t) {}
> 
> 
> and see what happens, if your code creates another record component when I think it shouldn't, etc. Actually you could try to override the type in the existing record component instead of creating a new one.
> 
> Another investigation you should do is how your code interacts with annotations (declaration and type anotations) on the record component. So in your test add annotations to the record component and see if you need to copy them etc. I think that if you override the type of the record component instead of creating a new one you should be fine with annotations, but not sure TBH

@vicente-romero-oracle thanks for your comment.

> I think that there are two test cases missing here. 

I added some tests about unknown type and annotation. Please take a look.

> Actually you could try to override the type in the existing record component instead of creating a new one.

I don't think that overriding the type in the existing record component can solve this issue. As we can see the constructor of the `RecordComponent`, the current fields `RecordComponent.type` and `RecordComponent.isVarargs` depend on the `type` of the argument `VarSymbol field`(`field.type`). So we need to revise `RecordComponent.type` and `RecordComponent.isVarargs` in the method `getRecordComponent`. If the `RecordComponent` adds new fields which depend on the `type` of the argument `VarSymbol field`(`field.type`) in the future, the developers need to revise the method `getRecordComponent` to override the new fields. It is uncomfortable and the code is hard to maintain. So I consider that creating a new one is a better way. 


        public RecordComponent(VarSymbol field, List<JCAnnotation> annotations) {
            super(PUBLIC, field.name, field.type, field.owner);   // <---- here uses `field.type`
            this.originalAnnos = annotations;
            this.pos = field.pos;
            /* it is better to store the original information for this one, instead of relying
             * on the info in the type of the symbol. This is because on the presence of APs
             * the symbol will be blown out and we won't be able to know if the original
             * record component was declared varargs or not.
             */
            this.isVarargs = type.hasTag(TypeTag.ARRAY) && ((ArrayType)type).isVarargs();  // <---- here `type` is `field.type`, too
        }

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java line 1514:
> 
>> 1512:                 if (rc.name == var.name && var.pos == rc.pos) {
>> 1513:                     if (rc.type.hasTag(TypeTag.ERROR) && !var.sym.type.hasTag(TypeTag.ERROR)) {
>> 1514:                         // Found a wrong record component: save it so that we can remove it later.
> 
> suggestion for the comment:
> 
> Found a record component with an erroneous type, save it so that it can be removed later.
> If the class type of the ...

Fixed.

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java line 1526:
> 
>> 1524:             RecordComponent rc = null;
>> 1525:             if (toRemove != null) {
>> 1526:                 // Found a wrong record component: remove it and create a new one.
> 
> suggestion:
> 
> Found a record component with an erroneous type, remove it and create a new one

Fixed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5511


More information about the compiler-dev mailing list