Set var arg type before calling `viewpointAdaptConstructor` #783

Ao-senXiong · 2024-06-11T14:15:50Z

Fixes #777

wmdietl

Thanks!

framework/tests/viewpointtest/VarargType.java

wmdietl

Having one passing and one failing test with actual viewpoint adaptation would give us much more confidence that the problem is actually fixed.

wmdietl · 2024-06-20T22:40:16Z

framework/tests/viewpointtest/VarargType.java

@@ -0,0 +1,27 @@
+import viewpointtest.quals.*;


Can you add the usual starter text with a link to the issue that this test is for?

Tests that actually use some annotations would be nice.

Can you add the usual starter text with a link to the issue that this test is for?

Done

Tests that actually use some annotations would be nice.

Done

wmdietl · 2024-06-20T22:40:54Z

framework/tests/viewpointtest/VarargType.java

@@ -0,0 +1,27 @@
+import viewpointtest.quals.*;
+
+public class VarargType {


What is the Type in the class name referring to?

changed it to VarargsConstructor.

wmdietl · 2024-06-20T22:44:27Z

framework/tests/viewpointtest/VarargType.java

+
+    // inner class
+    class Inner {
+        Inner(String str, Object... args) {}


How about only the varargs parameter and then test with:

new Inner()

new Inner(new Object())

Varargtype.this.new Inner()

Varargtype.this.new Inner(new Object())

To test more of the tricky combinations with inner classes.

wmdietl · 2024-06-20T22:45:48Z

framework/tests/viewpointtest/VarargType.java

+
+    // anonymous class
+    Object o =
+            new Object() {


This doesn't test that the varargs constructor can be used to create an anonymous class. It should be something like new VarargType(...) { /* either nothing or some code */ }

Okay, thanks!

Ao-senXiong · 2024-06-21T00:29:16Z

framework/tests/viewpointtest/VarargsConstructor.java

+
+    VarargsConstructor(String str, Object... args) {}
+
+    @SuppressWarnings({"inconsistent.constructor.type", "super.invocation.invalid"})


@wmdietl I feel like this is also happening when annotate the constructor in here

checker-framework/framework/tests/viewpointtest/PolyConstructor.java

Line 6 in 481040f

@SuppressWarnings({"inconsistent.constructor.type", "super.invocation.invalid"})

.

can you share some insight on these two error messages?

Do the full error messages not explain the errors well enough?
1.) Constructor type (%s) is a subtype of the top type (%s), therefore it cannot be statically verified. We cannot statically ensure that the constructor creates an object of a non-top type. Therefore, any constructor that is annotated with something non-top produces this.
2.) Constructor of type %s cannot call %s of type %s. The constructor from the superclass has the default annotation. The super constructor is implicitly called and this constructor has a different annotation - therefore, we get a warning about the mismatch.

Please open PRs to improve these error messages if something is unclear.

I notice that for these two errors there is no explanation in docs/manual.
We could improve our checks to:
1.) produce a list of all unique error keys from the messages.properties files.
2.) ensure each key appears somewhere in docs/manual. Ideally, outside of just a code example (e.g. one of the two error keys appears once as @SuppressWarnings("super.invocation.invalid"). Is that good enough as documentation?)
3.) ensure each key appears somewhere in the checker source code, to avoid that we have dead error messages in messages.properties files.
4.) ensure each key appears in at least one test case as expected error.

I'm not sure how well-covered the manual is currently for the error keys, but this looks like a nice goal to reach.
If this sounds useful, please convert this comment into an issue.

wmdietl

Thanks for the expansion!

wmdietl · 2024-06-21T19:18:51Z

framework/tests/viewpointtest/VarargsConstructor.java

+
+    VarargsConstructor(String str, Object... args) {}
+
+    @SuppressWarnings({"inconsistent.constructor.type", "super.invocation.invalid"})


Do the full error messages not explain the errors well enough?
1.) Constructor type (%s) is a subtype of the top type (%s), therefore it cannot be statically verified. We cannot statically ensure that the constructor creates an object of a non-top type. Therefore, any constructor that is annotated with something non-top produces this.
2.) Constructor of type %s cannot call %s of type %s. The constructor from the superclass has the default annotation. The super constructor is implicitly called and this constructor has a different annotation - therefore, we get a warning about the mismatch.

Please open PRs to improve these error messages if something is unclear.

I notice that for these two errors there is no explanation in docs/manual.
We could improve our checks to:
1.) produce a list of all unique error keys from the messages.properties files.
2.) ensure each key appears somewhere in docs/manual. Ideally, outside of just a code example (e.g. one of the two error keys appears once as @SuppressWarnings("super.invocation.invalid"). Is that good enough as documentation?)
3.) ensure each key appears somewhere in the checker source code, to avoid that we have dead error messages in messages.properties files.
4.) ensure each key appears in at least one test case as expected error.

I'm not sure how well-covered the manual is currently for the error keys, but this looks like a nice goal to reach.
If this sounds useful, please convert this comment into an issue.

framework/tests/viewpointtest/VarargsConstructor.java

Co-authored-by: Werner Dietl <wdietl@gmail.com>

Ao-senXiong · 2024-06-21T22:40:36Z

Submitted a new issue for error key coverage #789 .

wmdietl

Thanks!

Co-authored-by: Werner Dietl <wdietl@gmail.com>

Ao-senXiong added 3 commits June 11, 2024 10:13

Apply spotless

0aad74a

Use correct package path

c5f3f3c

Use different class name with annotation

816b787

Ao-senXiong requested a review from wmdietl June 17, 2024 14:43

Ao-senXiong assigned wmdietl Jun 17, 2024

Ao-senXiong mentioned this pull request Jun 19, 2024

Receiver of this() call is null #782

Open

wmdietl requested changes Jun 20, 2024

View reviewed changes

framework/tests/viewpointtest/VarargType.java Outdated Show resolved Hide resolved

framework/tests/viewpointtest/VarargType.java Outdated Show resolved Hide resolved

wmdietl assigned Ao-senXiong and unassigned wmdietl Jun 20, 2024

Refine test case and add inner & anonymous class test

3ab44c8

Ao-senXiong requested a review from wmdietl June 20, 2024 19:21

wmdietl requested changes Jun 20, 2024

View reviewed changes

Ao-senXiong added 2 commits June 20, 2024 20:24

More test cases for VarargsConstructor

14f4c61

Rename file to VarargsConstructor

b76f6b5

Ao-senXiong commented Jun 21, 2024

View reviewed changes

Ao-senXiong requested a review from wmdietl June 21, 2024 03:15

Ao-senXiong assigned wmdietl and unassigned Ao-senXiong Jun 21, 2024

wmdietl requested changes Jun 21, 2024

View reviewed changes

wmdietl assigned Ao-senXiong and unassigned wmdietl Jun 21, 2024

Ao-senXiong and others added 2 commits June 21, 2024 18:23

Apply suggestions from code review

70869f6

Co-authored-by: Werner Dietl <wdietl@gmail.com>

Update the test case with actual VPA for inner and anonymous class

884a1ec

Ao-senXiong mentioned this pull request Jun 21, 2024

Error key coverage in manual and messages.properties #789

Open

Ao-senXiong requested a review from wmdietl June 21, 2024 22:41

Ao-senXiong assigned wmdietl and unassigned Ao-senXiong Jun 22, 2024

wmdietl changed the title ~~Set var arg type before calling viewpointAdaptorConstructor~~ Set var arg type before calling viewpointAdaptConstructor Jun 23, 2024

wmdietl approved these changes Jun 23, 2024

View reviewed changes

wmdietl merged commit 4d8d1a5 into eisop:master Jun 23, 2024
39 checks passed

Ao-senXiong added a commit to Ao-senXiong/checker-framework that referenced this pull request Jun 23, 2024

Set var arg type before calling viewpointAdaptConstructor (eisop#783)

0d76a5a

Co-authored-by: Werner Dietl <wdietl@gmail.com>

Ao-senXiong added a commit to Ao-senXiong/checker-framework that referenced this pull request Jun 25, 2024

Set var arg type before calling viewpointAdaptConstructor (eisop#783)

65b1429

Co-authored-by: Werner Dietl <wdietl@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set var arg type before calling `viewpointAdaptConstructor` #783

Set var arg type before calling `viewpointAdaptConstructor` #783

Ao-senXiong commented Jun 11, 2024

wmdietl left a comment

wmdietl left a comment

wmdietl Jun 20, 2024

wmdietl Jun 20, 2024

Ao-senXiong Jun 21, 2024

wmdietl Jun 20, 2024

Ao-senXiong Jun 21, 2024

wmdietl Jun 20, 2024

Ao-senXiong Jun 21, 2024

wmdietl Jun 20, 2024

Ao-senXiong Jun 21, 2024

Ao-senXiong Jun 21, 2024

wmdietl Jun 21, 2024

wmdietl left a comment

wmdietl Jun 21, 2024

Ao-senXiong commented Jun 21, 2024

wmdietl left a comment

		@@ -0,0 +1,27 @@
		import viewpointtest.quals.*;

		public class VarargType {


		VarargsConstructor(String str, Object... args) {}

		@SuppressWarnings({"inconsistent.constructor.type", "super.invocation.invalid"})

Set var arg type before calling viewpointAdaptConstructor #783

Set var arg type before calling viewpointAdaptConstructor #783

Conversation

Ao-senXiong commented Jun 11, 2024

wmdietl left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

wmdietl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ao-senXiong commented Jun 21, 2024

wmdietl left a comment

Choose a reason for hiding this comment

Set var arg type before calling `viewpointAdaptConstructor` #783

Set var arg type before calling `viewpointAdaptConstructor` #783