Introduce `ClassMemberOrdering` bug checker

[benhalasi]

#595

Suggested commit message

Introduce `MemberOrder` check (#636)

[github-actions[bot]]

• Surviving mutants in this change: 23
• Killed mutants in this change: 63

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	23	63

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 23
• Killed mutants in this change: 63

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	23	63

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[benhalasi]

Verifying that the whitespace is untouched maybe a bit harder than I thought because TestMode.TEXT_MATCH formats both input and output.

I looked into doTest and TestMode and it doesn't seem too hard to just implement a new one that doesn't format, but I'll wait for some input before getting into it.

Indentation of replaced code might cause some challenge.

[github-actions[bot]]

• Surviving mutants in this change: 11
• Killed mutants in this change: 61

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	11	61

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 11
• Killed mutants in this change: 61

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	11	61

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 11
• Killed mutants in this change: 61

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	11	61

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[rickie]

(Okay apparently I had to rebuild, the test locally passed 😄, will take a look).

I didn't test if MemberOrdering respects annotations, I assumed annotations are part of the member Tree.

@benhalasi I would add a testcase with an annotation in that case 😉 .

I'm not sure how to approach verifying that this checker doesn't mess with whitespaces - as it's hard to test something it shouldn't do - I'm leaning towards to have a separate test method that has all the whitespace cases we want to test, which I'd need input on - and have the other cases properly formatted.

When using the suppression for ErrorProneTestHelperSourceFormat it should be fine to test the whitespaces right?

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

91.5% Coverage
0.0% Duplication

[github-actions[bot]]

• Surviving mutants in this change: 11
• Killed mutants in this change: 61

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	11	61

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[rickie]

I reverted the commit as I'd need some more time to properly fix this. I'm wondering however, maybe we should use a different method altogether. In the SourceCode file and in our recently moved StringLocaleUsage check (see here), we have examples of going over the ErrorProneTokens.getTokens. Maybe we should use that to get "the full Tree" that we need to replace. I'm not sure which direction is currently the best way to go in.

[github-actions[bot]]

• Surviving mutants in this change: 11
• Killed mutants in this change: 61

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	11	61

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 10
• Killed mutants in this change: 51

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	10	49
:tada:tech.picnic.errorprone.bugpatterns.MemberOrdering$MemberWithComments	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 10
• Killed mutants in this change: 51

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	10	49
:tada:tech.picnic.errorprone.bugpatterns.MemberOrdering$MemberWithComments	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 4
• Killed mutants in this change: 44

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	4	42
:tada:tech.picnic.errorprone.bugpatterns.MemberOrdering$ClassMemberWithComments	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[oxkitsune]

ErrorProneTokens#getTokens(String, Context) doesn't include comments as it relies on the start position of the provided member (e.g., the position of void). Comments don't count as tokens but do affect token positions.

To include comments, we can use VisitorState#getOffsetTokens(int, int) to obtain tokens in a specific range. By starting the range at the end position of the previous token and ending it at the end of the current member, we'll obtain the same tokens, now including comments in the resulting ErrorProneToken.

[github-actions[bot]]

• Surviving mutants in this change: 4
• Killed mutants in this change: 44

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	4	42
:tada:tech.picnic.errorprone.bugpatterns.MemberOrdering$ClassMemberWithComments	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 4
• Killed mutants in this change: 44

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	4	42
:tada:tech.picnic.errorprone.bugpatterns.MemberOrdering$ClassMemberWithComments	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 5
• Killed mutants in this change: 50

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MemberOrdering	5	48
:tada:tech.picnic.errorprone.bugpatterns.MemberOrdering$ClassMemberWithComments	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[benhalasi]

Thanks oxkitsune for the ErrorProneTokens#getTokens(String, Context)!

Apart from the minor questions below, the only big thing to tackle is verifying that this checker reserves whitespaces or deciding that we don't verify it.

[github-actions[bot]]

• Surviving mutants in this change: 3
• Killed mutants in this change: 49

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.ClassMemberOrdering	3	47
:tada:tech.picnic.errorprone.bugpatterns.ClassMemberOrdering$ClassMemberWithComments	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 2
• Killed mutants in this change: 50

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.ClassMemberOrdering	2	48
:tada:tech.picnic.errorprone.bugpatterns.ClassMemberOrdering$ClassMemberWithComments	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[oxkitsune]

Looks good!

However blocks (both static and instance) are also considered class members:

class Foo {
	private static final int bar;
	private final int baz;


	// some static initializer block
	static {
		bar = 110 ^ 67;
	}

	// some instance initializer block
	{
		baz = (1395 ^ 1059) >> 3;
	}
}

Currently these are not sorted, I'm unsure what their order should be though. Something like this perhaps?

• static fields
• static blocks
• non-static fields
• non-static blocks

WDYT :smile:

[benhalasi]

However blocks (both static and instance) are also considered class members: ...

Good point, they're indeed non-handled. My reasoning to ignore this case was that using static blocks are very rare and specific. It would be hard to put these very specific edge cases into a rule, if even possible. Seems more practical to not sort them in this rule (or at all).

Afaik the general opinion on static blocks is that we should just not use them. Considering this, (imo) if we want to do anything with static blocks in error-prone, we should just flag them as errors.

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

94.4% Coverage
0.0% Duplication

[github-actions[bot]]

• Surviving mutants in this change: 5
• Killed mutants in this change: 48

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.ClassMemberOrdering	5	48

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

94.4% Coverage
0.0% Duplication

[github-actions[bot]]

• Surviving mutants in this change: 5
• Killed mutants in this change: 48

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.ClassMemberOrdering	5	48

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[benhalasi]

Hehe, I'll check it out soon (this week 🤞) and will update it with the open questions!

[benhalasi]

Seems PiTest in CI consistently fails in ~1m w/

Failed to execute goal org.pitest:pitest-maven:1.15.3:mutationCoverage (default-cli) on project error-prone-contrib: Execution default-cli of goal org.pitest:pitest-maven:1.15.3:mutationCoverage failed: Coverage generation minion exited abnormally! (UNKNOWN_ERROR)

Didn't deep-dive in its logs nor tried executing it locally yet, just flagging!

[Stephan202]

Seems PiTest in CI consistently fails in ~1m w/

Failed to execute goal org.pitest:pitest-maven:1.15.3:mutationCoverage (default-cli) on project error-prone-contrib: Execution default-cli of goal org.pitest:pitest-maven:1.15.3:mutationCoverage failed: Coverage generation minion exited abnormally! (UNKNOWN_ERROR)

Didn't deep-dive in its logs nor tried executing it locally yet, just flagging!

If you rebase this should be fixed, thanks to #976 :)

W.r.t. other questions: I'll have to get back to you, hopefully later this week. Tnx for reviving this PR!