Add baseline support for records #3017

michaelhixson · 2019-12-25T17:31:01Z

Treat records as regular classes. Avoid throwing exceptions when records
are encounterd in source code. Keep backwards compatibility with Java 8
and 11.

Related to #3014

Treat records as regular classes. Avoid throwing exceptions when records are encounterd in source code. Keep backwards compatibility with Java 8 and 11.

mernst · 2019-12-26T00:31:55Z

Thanks for the pull request. CI is failing because the changes don't compile. Could you please correct that and get the CI jobs passing? Thanks! Ask for help if you get stuck.

michaelhixson · 2019-12-26T04:03:28Z

I'm trying to understand the latest build failure in misc_jdk8/test-misc.sh. Is it telling me to add javadoc to these methods? If so, why?

/__w/1/s/javacutil/src/main/java/org/checkerframework/javacutil/ElementUtils.java:495: missing documentation for isTypeDeclaration
/__w/1/s/javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java:820: missing documentation for classTreeKinds

##[error]Bash exited with code '1'.
Finishing: test-misc.sh

I reverted the changes outside of dataflow and javacutil, since I realized I'm only using those two and can't vouch for any other changes.

mernst · 2019-12-26T04:42:57Z

@michaelhixson The rule requires documentation for anything that a pull request affects. The rule includes any lines adjacent to ones changed in the pull request. I added documentation for those two lines.

michaelhixson · 2019-12-26T15:32:44Z

I'll try to figure out if I caused the latest build errors and/or if I can fix them.

Also, I have a couple of questions about code that I'm not sure if I should change.

1. Resolver.findClassInPackage

Currently, Resolver#findClassInPackage(String, PackageSymbol, TreePath) finds an element and checks if its kind is exactly ElementKind.CLASS. It looks suspicious to me.

checker-framework/javacutil/src/main/java/org/checkerframework/javacutil/Resolver.java

Lines 278 to 289 in cff1a03

    
           Element res = 
        
                   wrapInvocationOnResolveInstance( 
        
                           FIND_IDENT_IN_PACKAGE, 
        
                           env, 
        
                           pck, 
        
                           names.fromString(name), 
        
                           Kinds.KindSelector.TYP); 
        
           if (res.getKind() == ElementKind.CLASS) { 
        
               return (ClassSymbol) res; 
        
           } else { 
        
               return null; 
        
           }

Here is that method's only caller.

checker-framework/framework/src/main/java/org/checkerframework/framework/util/FlowExpressionParseUtil.java

Lines 494 to 498 in cff1a03

    
           ClassSymbol classSymbol = 
        
                   resolver.findClassInPackage(expr.getNameAsString(), packageSymbol, path); 
        
           if (classSymbol != null) { 
        
               return new ClassName(classSymbol.asType()); 
        
           }

Should that check in findClassInPackage be changed to one of these instead?

if (res.getKind().isClass() || res.getKind().isInterface()) {
    return (ClassSymbol) res;

if (res instanceof ClassSymbol) {
    return (ClassSymbol) res;

2. AnnotationUtils.getElementKindsForElementType

Currently, AnnotationUtils#getElementKindsForElementType(ElementType) contains code to accomodate Java 9 modules. It uses a different strategy than the code I added for records, so I'm wondering if I should modify the existing code, my code, or neither.

checker-framework/javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java

Lines 556 to 560 in cff1a03

    
           // TODO: Add actual case to check for the enum constant and return Set containing 
        
           // ElementKind.MODULE.  (Java 11) 
        
           if (elementType.name().contentEquals("MODULE")) { 
        
               return EnumSet.noneOf(ElementKind.class); 
        
           }

In this PR, I added the following code right below that.

if (elementType.name().equals("RECORD_COMPONENT")) {
    return EnumSet.of(ElementKind.valueOf("RECORD_COMPONENT"));
}

Shouldn't the module-handling code be changed? If the caller is passing ElementType.MODULE, that implies that ElementKind.MODULE also exists, doesn't it?

if (elementType.name().contentEquals("MODULE")) {
    return EnumSet.of(ElementKind.valueOf("MODULE"));
}

wmdietl

Thanks for these fixes! I have a few small comments, but this looks good.

Regarding the first question about Resolver.findClassInPackage I agree that that looks strange. Could you file an issue to fix and/or document this?

I've answered Q2 in the review.

wmdietl · 2019-12-26T17:36:52Z

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/ClassNameNode.java

-                || tree.getKind() == Tree.Kind.ENUM
-                || tree.getKind() == Tree.Kind.INTERFACE
-                || tree.getKind() == Tree.Kind.ANNOTATION_TYPE;
+        assert TreeUtils.isClassTree(tree);


I think we should just remove this assert. The parameter is a ClassTree and I don't see what this is trying to guard against.

wmdietl · 2019-12-26T18:04:24Z

dataflow/src/main/java/org/checkerframework/dataflow/analysis/FlowExpressions.java

            case ENUM_CONSTANT:
            case FIELD:
                TypeMirror fieldType = TreeUtils.typeOf(memberSelectTree);
                Receiver r = internalReprOf(provider, memberSelectTree.getExpression());
                return new FieldAccess(r, fieldType, (VariableElement) ele);
            default:
+                if (ele.getKind().isClass() || ele.getKind().isInterface()) {
+                    TypeMirror selectType = TreeUtils.typeOf(memberSelectTree);


Could you copy over the two comments from the previous cases?

wmdietl · 2019-12-26T18:06:32Z

javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java

-                        ElementKind.INTERFACE,
-                        ElementKind.ANNOTATION_TYPE,
-                        ElementKind.ENUM);
+                EnumSet<ElementKind> typeKinds = EnumSet.noneOf(ElementKind.class);


I'm wondering whether it's worth to also extract this into a field, like is done for trees.
This method isn't on a hot path as far as I can tell, but having the symmetry and separate code might still improve readability of the code.
What do you think?

This sounds reasonable. I'm wondering how far I should take the symmetry.

TreeUtils has:

private static final Set<Tree.Kind> classTreeKinds public static Set<Tree.Kind> classTreeKinds() public static boolean isClassTree(Tree)

I could add these to ElementUtils:

private static final Set<ElementKind> classElementKinds public static Set<ElementKind> classElementKinds() public static boolean isClassElement(Element)

and then I could refactor the places currently doing element.getKind().isClass() || element.getKind().isInterface() to instead do ElementUtils.isClassElement(element).

Does that sound worthwhile, or is that going too far?

Oops, one minor correction. I would need to specify EnumSet as the type of ElementUtils.classElementKinds if I wanted to return it from AnnotationUtils.getElementKindsForElementType. That, or return EnumSet.copyOf(ElementUtils.classElementKinds()) (which would take an optimized path in EnumSet.copyOf(...), since the collection it's copying also happens to be an EnumSet).

That sounds worthwhile. Using EnumSet sounds good and would probably also be good for the Tree.Kind sets. Also, we should make defensive copies to prevent somebody from modifying the definitions of these sets.
Feel free to open an issue for further cleanups that aren't relevant for this PR.
Thanks!

Mind taking another look? I did not end up changing the static set types from Set to EnumSet or making defensive copies, because that was spiraling out into too many unrelated changes (changing other similar sets in TreeUtils, changing internal callers to avoid the defensive copies, but realizing there are no external callers and so no one needs the defensive copy, and so on). IMHO it's ok as is, but let me know if you disagree.

wmdietl · 2019-12-26T18:09:05Z

javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java

@@ -558,6 +560,9 @@ public static boolean hasInheritedMeta(AnnotationMirror anno) {
                if (elementType.name().contentEquals("MODULE")) {
                    return EnumSet.noneOf(ElementKind.class);


Regarding your second question: 3ef0af3 added this work-around.
I'm not quite sure why we need it, as we use at least a Java 9 compiler, so using MODULE should always work.
I agree that using ElementKind.valueOf("MODULE") should also work here.
Could you try that as a separate PR?

Thanks for filing #3019.

wmdietl · 2019-12-26T18:11:22Z

javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java

-            default:
-                return false;
-        }
+        // These tree kinds are always declarations.  Uses of the declared types


You removed this comment in the Element version of this method. I agree that the comment doesn't add anything useful, so I would be fine with removing it here too.

Provides symmetry with classTreeKinds and isClassTree of TreeUtils.

michaelhixson · 2019-12-26T20:57:53Z

@wmdietl I think I addressed all the feedback.

wmdietl

Thanks for these improvements! It's great that we can support record without any version-specific code.

wmdietl · 2019-12-31T10:58:39Z

javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java

@@ -558,6 +560,9 @@ public static boolean hasInheritedMeta(AnnotationMirror anno) {
                if (elementType.name().contentEquals("MODULE")) {
                    return EnumSet.noneOf(ElementKind.class);


Thanks for filing #3019.

Add baseline support for records

9449ea4

Treat records as regular classes. Avoid throwing exceptions when records are encounterd in source code. Keep backwards compatibility with Java 8 and 11.

michaelhixson mentioned this pull request Dec 25, 2019

Support records #3014

Closed

michaelhixson added 5 commits December 25, 2019 18:40

Revert changes outside of javacutil and dataflow

b5d9b46

Restore TYPE_ANNOTATION case, which was accidentally removed

4c1ba95

Revert accidental changes

5d5ae40

Revert accidental changes

44679dd

Try fixing code style issues

d7403cf

mernst added 2 commits December 25, 2019 20:41

Add documentation

7f4b4ca

Merge ../checker-framework-branch-master into support-records

d0e1808

Reuse utility methods

469e846

michaelhixson added 3 commits December 26, 2019 07:48

Use a utility method

dcd4c4c

Fix AnnotationUtils.getElementKindsForElementType

85b30f1

Dynamically detect class tree kinds, avoid hardcoded enum set

a41a532

wmdietl self-assigned this Dec 26, 2019

michaelhixson and others added 3 commits December 26, 2019 08:40

See if this is what the code formatter is complaining about

bfb4b42

Add javadocs

bc8371b

Move field initialization together.

1aa4234

wmdietl requested changes Dec 26, 2019

View reviewed changes

wmdietl assigned michaelhixson and unassigned wmdietl Dec 26, 2019

michaelhixson mentioned this pull request Dec 26, 2019

Suspicious check in Resolver.findClassInPackage #3018

Closed

michaelhixson added 4 commits December 26, 2019 11:19

Review feedback: Remove unwanted comment

8433f13

Review feedback: Remove useless assertion

bfe2367

Add classElementKinds and isClassElement to ElementUtils

980870b

Provides symmetry with classTreeKinds and isClassTree of TreeUtils.

Use new ElementUtils methods

94916c7

michaelhixson mentioned this pull request Dec 26, 2019

Resolve TODO in AnnotationUtils to return ElementKind.MODULE #3019

Merged

michaelhixson removed their assignment Dec 26, 2019

wmdietl approved these changes Dec 31, 2019

View reviewed changes

wmdietl merged commit e2874e7 into typetools:master Dec 31, 2019

michaelhixson mentioned this pull request Jan 5, 2020

Support records uber/NullAway#377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add baseline support for records #3017

Add baseline support for records #3017

michaelhixson commented Dec 25, 2019

mernst commented Dec 26, 2019

michaelhixson commented Dec 26, 2019

mernst commented Dec 26, 2019

michaelhixson commented Dec 26, 2019 •

edited

wmdietl left a comment

wmdietl Dec 26, 2019

wmdietl Dec 26, 2019

wmdietl Dec 26, 2019

michaelhixson Dec 26, 2019

michaelhixson Dec 26, 2019

wmdietl Dec 26, 2019

michaelhixson Dec 26, 2019

wmdietl Dec 26, 2019

wmdietl Dec 31, 2019

wmdietl Dec 26, 2019

michaelhixson commented Dec 26, 2019

wmdietl left a comment

wmdietl Dec 31, 2019

		@@ -558,6 +560,9 @@ public static boolean hasInheritedMeta(AnnotationMirror anno) {
		if (elementType.name().contentEquals("MODULE")) {
		return EnumSet.noneOf(ElementKind.class);

Add baseline support for records #3017

Add baseline support for records #3017

Conversation

michaelhixson commented Dec 25, 2019

mernst commented Dec 26, 2019

michaelhixson commented Dec 26, 2019

mernst commented Dec 26, 2019

michaelhixson commented Dec 26, 2019 • edited

1. Resolver.findClassInPackage

2. AnnotationUtils.getElementKindsForElementType

wmdietl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelhixson commented Dec 26, 2019

wmdietl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelhixson commented Dec 26, 2019 •

edited