Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #12210: Add method to ignore unstable checker framework violations #12215

Merged
merged 1 commit into from
Sep 19, 2022
+117 −31
Merged

Issue #12210: Add method to ignore unstable checker framework violations #12215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion .ci/StarterRuleSet-AllRulesByCategory.groovy.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,12 @@ ruleset {
EmptyMethodInAbstractClass
FinalClassWithProtectedMember
ImplementationAsType
Instanceof

// Using instanceof in equals method is alright
Instanceof {
ignoreTypeNames = 'CheckerFrameworkError'
}

LocaleSetDefault
NestedForLoop
PrivateFieldCouldBeFinal
Expand Down
63 changes: 42 additions & 21 deletions .ci/checker-framework-suppressions/checker-framework-methods-resource-fenum-suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/Main.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-10288 or any of its aliases.</message>
Expand Down Expand Up @@ -77,7 +78,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20912 or any of its aliases.</message>
Expand All @@ -88,7 +90,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20915 or any of its aliases.</message>
Expand All @@ -99,7 +102,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20917 or any of its aliases.</message>
Expand All @@ -110,7 +114,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20918 or any of its aliases.</message>
Expand All @@ -121,7 +126,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20923 or any of its aliases.</message>
Expand All @@ -132,7 +138,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20926 or any of its aliases.</message>
Expand Down Expand Up @@ -176,7 +183,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/meta/XmlMetaReader.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-23288 or any of its aliases.</message>
Expand All @@ -198,7 +206,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/DescendantIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20533 or any of its aliases.</message>
Expand All @@ -209,7 +218,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/DescendantIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20534 or any of its aliases.</message>
Expand All @@ -220,7 +230,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/DescendantIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20536 or any of its aliases.</message>
Expand All @@ -231,7 +242,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/FollowingIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20414 or any of its aliases.</message>
Expand All @@ -242,7 +254,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/FollowingIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20415 or any of its aliases.</message>
Expand All @@ -253,7 +266,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/FollowingIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20418 or any of its aliases.</message>
Expand All @@ -264,7 +278,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/FollowingIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-20420 or any of its aliases.</message>
Expand All @@ -275,7 +290,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/PrecedingIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-10345 or any of its aliases.</message>
Expand All @@ -286,7 +302,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/PrecedingIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-10346 or any of its aliases.</message>
Expand All @@ -297,7 +314,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/PrecedingIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-10349 or any of its aliases.</message>
Expand All @@ -309,7 +327,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/PrecedingIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-10351 or any of its aliases.</message>
Expand All @@ -320,7 +339,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/ReverseDescendantIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-22037 or any of its aliases.</message>
Expand All @@ -331,7 +351,8 @@
</details>
</checkerFrameworkError>

<checkerFrameworkError unstable="false">
<!-- Error is considered unstable as temp-var changes with each run. -->
<checkerFrameworkError unstable="true">
<fileName>checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/xpath/iterators/ReverseDescendantIterator.java</fileName>
<specifier>required.method.not.called</specifier>
<message>@MustCall method close may not have been invoked on temp-var-22043 or any of its aliases.</message>
Expand Down
78 changes: 69 additions & 9 deletions .ci/checker-framework.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import groovy.transform.EqualsAndHashCode
import groovy.transform.Field
import groovy.transform.Immutable
import groovy.util.slurpersupport.GPathResult
Expand Down Expand Up @@ -382,7 +381,6 @@ private static Set<CheckerFrameworkError> setDifference(final Set<CheckerFramewo
/**
* A class to represent the XML {@code checkerFrameworkError} node.
*/
@EqualsAndHashCode(excludes = ["lineNumber", "unstable"])
@Immutable
class CheckerFrameworkError implements Comparable<CheckerFrameworkError> {

Expand All @@ -398,6 +396,17 @@ class CheckerFrameworkError implements Comparable<CheckerFrameworkError> {
List<String> details
String lineContent
int lineNumber

/**
* Whether the error is unstable. Unstable errors in suppression list are not flagged as
nrmancuso marked this conversation as resolved.
Show resolved Hide resolved
* unnecessary suppressions.
*
* <p>An error is considered to be unstable when error message changes with each run.
* Some error messages contains strings like {@code temp-var-1234}, the numerical part changes
* with each run so the error is considered unstable. In such cases numerical values in
* {@code details} and {@code message} are replaced with empty string while comparing and
* hashing errors.
*/
boolean unstable

@Override
Expand Down Expand Up @@ -435,17 +444,68 @@ class CheckerFrameworkError implements Comparable<CheckerFrameworkError> {
return i
}

i = getMessage() <=> other.getMessage()
if (i != 0) {
return i
if (this.isUnstable() || other.isUnstable()) {
final String messageWithoutLineNumber = getMessage().replaceAll('\\d+', '')
final String thatMessageWithoutLineNumber = other.getMessage().replaceAll('\\d+', '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn't we do this in the constructor? This is suppose to be an immutable class.

Would also save on execution time, as this gets run probably every time a new entry gets added to the set for nearly everything in the set multiple times.

We obviously don't care about the numbers anyways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is suppose to be an immutable class

It still is, the original field is not modified.

Would also save on execution time, as this gets run probably every time a new entry gets added to the set for nearly everything in the set multiple times.

It will only run for unstable mutations, only a few of them are there.

We obviously don't care about the numbers anyways.

I feel it would be better if we don't modify the original error itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will numbers that were an issue always be 3 digits? Maybe consider changing the regexp to just match 3 digit numbers.
Also why not match the # too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will numbers that were an issue always be 3 digits?

No, they are not, see the updated issue description, we have entries like temp-var-20910

Also why not match the # too?

Not possible as we have entries like temp-var-20910, also if we could do it, there is no point in doing it, would increase the complexity of regex unnecessarily.

i = messageWithoutLineNumber <=> thatMessageWithoutLineNumber
if (i != 0) {
return i
}

final List<String> detailsWithoutLineNumber = this.getDetails()*.replaceAll('\\d+', '')
final List<String> thatDetailsWithoutLineNumber =
other.getDetails()*.replaceAll('\\d+', '')

i = detailsWithoutLineNumber.join('') <=> thatDetailsWithoutLineNumber.join('')
if (i != 0) {
return i
}
}
else {
i = getMessage() <=> other.getMessage()
if (i != 0) {
return i
}

i = getLineContent() <=> other.getLineContent()
if (i != 0) {
return i
i = getDetails().join('') <=> other.getDetails().join('')
if (i != 0) {
return i
}
}

return getLineContent() <=> other.getLineContent()
}

@Override
boolean equals(Object object) {
if (this.is(object)) {
return true
}
if (!(object instanceof CheckerFrameworkError)) {
return false
}

return this.getDetails().join('') <=> other.getDetails().join('')
CheckerFrameworkError that = (CheckerFrameworkError) object

return (this <=> that) == 0
}

@Override
int hashCode() {
int result
if (unstable) {
result = (message != null ? message.replaceAll('\\d+', '').hashCode() : 0)
result = 31 * result + (details != null ? details*.replaceAll(
'\\d+', '').hashCode() : 0)
}
else {
result = (message != null ? message.hashCode() : 0)
result = 31 * result + (details != null ? details.hashCode() : 0)
}
result = 31 * result + (fileName != null ? fileName.hashCode() : 0)
result = 31 * result + (specifier != null ? specifier.hashCode() : 0)
result = 31 * result + (lineContent != null ? lineContent.hashCode() : 0)
return result
}

/**
Expand Down