Skip to content

Issue #18494: Add GoogleNonConstantFieldNameCheck for Google Java Style compliance#18657

Open
vivek-0509 wants to merge 1 commit intocheckstyle:masterfrom
vivek-0509:addingNewGoogleMemberNameCheck
Open

Issue #18494: Add GoogleNonConstantFieldNameCheck for Google Java Style compliance#18657
vivek-0509 wants to merge 1 commit intocheckstyle:masterfrom
vivek-0509:addingNewGoogleMemberNameCheck

Conversation

@vivek-0509
Copy link
Contributor

@vivek-0509 vivek-0509 commented Jan 15, 2026

Adds GoogleNonConstantFieldNameCheck to enforce non-constant field naming per Google Java Style Guide §5.2.5.

Part of #17842
Detailed Issue at #18494

Rules Enforced

  • lowerCamelCase format required
  • Single-character names rejected (f)
  • Hungarian-style prefixes rejected (mValue, sCount)
  • Underscores only between adjacent digits (guava33_4_5 )
  • No leading/trailing/consecutive underscores

Scope

  • This check validates instance fields, while excluding constants (static final), static non-final fields and local variables.

Diff Regression config: https://gist.githubusercontent.com/vivek-0509/36cbe7f3789730951f15e7b6d551c6f3/raw/7ac2050528756f02e9952ae546a9c1e8cfed29c0/base_config.xml

Diff Regression patch config: https://gist.githubusercontent.com/vivek-0509/a979757b15933ce44f9b0f676a038195/raw/77433e7a8e91c9d621be184f4efcb829cb12d868/patch_config.xml

Contirbution repo PR: checkstyle/contribution#1021

@vivek-0509
Copy link
Contributor Author

GitHub, generate report

@vivek-0509
Copy link
Contributor Author

Github, generate site

@github-actions
Copy link
Contributor

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/21036868447

@vivek-0509 vivek-0509 marked this pull request as draft January 15, 2026 16:13
@vivek-0509 vivek-0509 force-pushed the addingNewGoogleMemberNameCheck branch from 2508be9 to 0621ade Compare January 15, 2026 16:39
@vivek-0509
Copy link
Contributor Author

GitHub, generate report

@github-actions
Copy link
Contributor

@vivek-0509
Copy link
Contributor Author

Hey @romani, I have reviewed the diff report all are genuine violations.
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/0621ade_2026170816/reports/diff/index.html

@vivek-0509 vivek-0509 marked this pull request as ready for review January 15, 2026 18:10
@vivek-0509 vivek-0509 changed the title Issue #17842: Add GoogleMemberNameCheck for Google Java Style compliance Issue #18494: Add GoogleMemberNameCheck for Google Java Style compliance Jan 15, 2026
@vivek-0509 vivek-0509 force-pushed the addingNewGoogleMemberNameCheck branch from 0621ade to 649199d Compare January 15, 2026 20:41
@romani
Copy link
Member

romani commented Jan 16, 2026

Diff report is very huge hard to get actual diff.
Please generate report in comparison to old module that covered this to catch actual diff. Messages likely should be overriden to be same to avoid diff in wording to be a reason of diff.

@vivek-0509
Copy link
Contributor Author

GitHub, generate report

@github-actions
Copy link
Contributor

@vivek-0509
Copy link
Contributor Author

vivek-0509 commented Jan 16, 2026

Hi @romani , I have generated a comparison diff report. I have overridden the messages to be identical. Here is the report https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/649199d_2026065114/reports/diff/index.html. Please let me know if I have understood your request correctly or if any changes are needed.

@romani
Copy link
Member

romani commented Jan 16, 2026

diff is still wild.
please give same id to both Checks to try avoid diff detection that is purely due to name of Check.

@vivek-0509
Copy link
Contributor Author

GitHub, generate report

@github-actions
Copy link
Contributor

@vivek-0509
Copy link
Contributor Author

@romani I updated the configurations to use the same id for both checks, but they still appear as Red/Green pairs. It seems the report tool uses the full Check class name for the rule name, so MemberName and GoogleMemberName are treated as different rules despite the shared ID. Am I missing something?

@vivek-0509
Copy link
Contributor Author

@romani
I was investigating the report generation tool and i find that currently, the diff report compares violations using the full source string in
https://github.com/checkstyle/contribution/blob/4ac81f9f9928d695869b93c568fced0d98a1e781/patch-diff-report-tool/src/main/java/com/github/checkstyle/data/CheckstyleRecord.java#L249-L252

When an explicit id is set in the config, the ID is appended to the class name in the format ClassName#id (e.g., MemberNameCheck#GoogleMemberNameDiff). This means when two different checks share the same id, their source strings still differ because of the class name prefix (e.g., MemberNameCheck#GoogleMemberNameDiff vs GoogleMemberNameCheck#GoogleMemberNameDiff). As a result, violations with identical line, column, severity, and message are incorrectly reported as "removed" from base and "added" in patch.

the fix can be that, If source contains # (indicating an explicit module ID was set), compare only the portion after # instead of the full string. This would allow proper matching of violations from different checks that share the same ID, and the diff report would show only true behavioral differences.

@romani
Copy link
Member

romani commented Jan 17, 2026

tools is right, we tried to hack it, but tool is right in it behavior to show diff.

@romani
Copy link
Member

romani commented Jan 17, 2026

so, we can look at diff and see why we have more violations that it used to be.

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/649199d_2026150917/reports/diff/elasticsearch/index.html#A1
is good example, previosly we did not do violation, now we do.
https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names

google state that type immutability if critical point in decision. Checkstyle is not type aware, we can not make decision if it is immutable or not. So better to not give violation.
False positive will make users be frustrated, as it will block them, but false-negative (absence of violation) will not make users angry.

@vivek-0509
Copy link
Contributor Author

Understood, will change the scope to skip all static fields and update the issue accordingly.

@vivek-0509 vivek-0509 force-pushed the addingNewGoogleMemberNameCheck branch from 649199d to f342244 Compare January 17, 2026 10:03
@vivek-0509
Copy link
Contributor Author

GitHub, generate report

@github-actions
Copy link
Contributor

@vivek-0509
Copy link
Contributor Author

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

items:

* for word boundaries.</li>
* <li>Underscores may be used to separate adjacent numbers (e.g., version
* numbers like {@code guava33_4_5}), but NOT between letters and digits.</li>
* </ul>
Copy link
Member

Choose a reason for hiding this comment

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

we need to mention that static fields are skipped, as Chckstyle not able to make sure that used type is immutable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* @since 13.1.0
*/
@StatelessCheck
public class GoogleMemberNameCheck extends AbstractCheck {
Copy link
Member

Choose a reason for hiding this comment

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

Checks that member names conform to the

Google Java Style Guide
for non-constant field naming.

hmm, why we named it GoogleMemberNameCheck ?

as it should be GoogleNonConstantFieldNameCheck.
and it will make it clear why we have limitation on static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@vivek-0509 vivek-0509 force-pushed the addingNewGoogleMemberNameCheck branch from f342244 to 4024f27 Compare January 18, 2026 12:19
@vivek-0509 vivek-0509 force-pushed the addingNewGoogleMemberNameCheck branch from 0bb782c to 3bf4c23 Compare February 6, 2026 18:35
@vivek-0509
Copy link
Contributor Author

GitHub, generate website

@vivek-0509
Copy link
Contributor Author

@Zopsss All the changes done please review
the diff report link https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/0bb782c_2026121113/reports/diff/index.html

Copy link
Member

@Zopsss Zopsss left a comment

Choose a reason for hiding this comment

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

minors:

class Example2 {

static final int STATIC_FINAL = 0;
private static final int Invalid_Name = 1;
Copy link
Member

Choose a reason for hiding this comment

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

please correct the variable name. It is static final so it doesn't throw any violation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* @param ast the VARIABLE_DEF AST node
* @return true if this variable should be checked
*/
private static boolean mustCheckName(DetailAST ast) {
Copy link
Member

Choose a reason for hiding this comment

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

please rename this method to shouldCheckFieldName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 150 to 158
if (INVALID_UNDERSCORE_PATTERN.matcher(fieldName).find()) {
log(nameAst, MSG_KEY_INVALID_UNDERSCORE, fieldName);
}
else {
final String nameWithoutNumberingSuffix = NUMBERING_SUFFIX_PATTERN
.matcher(fieldName).replaceAll("");
if (!NON_CONSTANT_FIELD_NAME_PATTERN.matcher(nameWithoutNumberingSuffix).matches()) {
log(nameAst, MSG_KEY_INVALID_FORMAT, fieldName);
}
Copy link
Member

Choose a reason for hiding this comment

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

here we are using if/else condition.

If there is a variable like aTest_Test_09_90 then on the first pass it will log violation for invalid underscore, then user will run CS again and it will log violation for invalid non constant field name.

User will need to run CS 2 times.

The user should be able to solve all problems in one go, for that all violations should be logged in the first pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will use a single message and will simplify the method, the message would be

Non-constant field name ''{0}'' must start lowercase, be at least 2 chars, avoid single lowercase letter followed by uppercase, contain only letters, digits or underscores, with underscores allowed only between adjacent digits.

This way, when a name fails validation, the user sees all the rules that are enforced and can fix everything in one go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@vivek-0509 vivek-0509 force-pushed the addingNewGoogleMemberNameCheck branch from 3bf4c23 to 60f0289 Compare February 8, 2026 01:11
@vivek-0509
Copy link
Contributor Author

GitHub, generate website

@vivek-0509
Copy link
Contributor Author

GitHub, generate report

@vivek-0509
Copy link
Contributor Author

@Zopsss All the changes are done please review

@vivek-0509 vivek-0509 requested a review from Zopsss February 8, 2026 01:37
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2026

@vivek-0509
Copy link
Contributor Author

@Zopsss Ping

@Zopsss
Copy link
Member

Zopsss commented Feb 13, 2026

@vivek-0509 There is different number of violations in old diff report ( where we had multiple violation messages ) and the latest diff report ( where we have only one violation message )

Old report: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/0bb782c_2026121113/reports/diff/index.html

Latest Report: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/60f0289_2026020714/reports/diff/index.html

HBase & openjdk25's violation messages numbers do not match. Can you investigate why this happened? And did we introduced any false-positive/negative with new changes?

@vivek-0509
Copy link
Contributor Author

@Zopsss I also noticed this change. However, there are no logical changes or modifications to the regex. We only consolidated the if else blocks into a single condition while preserving the same logic in the check class.

I re-reviewed the diff report after the changes with this in mind and did not find any false positives. Since we fetch the latest code, the increase in violations appears to be due to new files added to the codebase that already contain these violations.

In the diff report, the difference is observed for Checkstyle and OpenJDK, while HBase still shows the same number of violations.

@vivek-0509
Copy link
Contributor Author

vivek-0509 commented Feb 13, 2026

@Zopsss
For checkstyle in the old report we see 715 files were checked
Screenshot 2026-02-13 at 12 02 39 PM

In new report there are 721 files checked
Screenshot 2026-02-13 at 12 02 50 PM

So the new files also has violations that our check flagged due to which we see increased in the violation number.

Same case with openJDK there also 2 files increased which lead to increase in the violation number

@vivek-0509
Copy link
Contributor Author

For Checkstyle, I found a new file named InputXpathNumericalPrefixesInfixesSuffixesCharacterCaseOne.java, InputXpathNumericalPrefixesInfixesSuffixesCharacterCaseTwo.java that was not present in the previous report. This file contains genuine violations, which explains the increase in the number of violations.

Link to one of the new files in the new diff report
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/60f0289_2026020714/reports/diff/checkstyle/index.html#A214

@Zopsss
Copy link
Member

Zopsss commented Feb 13, 2026

@vivek-0509 if you have extra time, thne can you also check reason behind extra violations in openjdk? If you're not able to then it's also fine.

1 similar comment
@Zopsss
Copy link
Member

Zopsss commented Feb 13, 2026

@vivek-0509 if you have extra time, thne can you also check reason behind extra violations in openjdk? If you're not able to then it's also fine.

@vivek-0509
Copy link
Contributor Author

vivek-0509 commented Feb 13, 2026

I have also checked for openJDK before sorry for not mentioning about it, it has the same reason
for old report 3980 files were checked
Screenshot 2026-02-13 at 11 22 41 PM

for New report 3982 files are checked
Screenshot 2026-02-13 at 11 23 08 PM

which resulted in the increase of violation count by 7

@vivek-0509
Copy link
Contributor Author

@Zopsss
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/60f0289_2026020714/reports/diff/openjdk25/index.html#A493
one example of such new file which was not there in the old report

@vivek-0509
Copy link
Contributor Author

vivek-0509 commented Feb 13, 2026

TestPhaseIRMatching.java
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/60f0289_2026020714/reports/diff/openjdk25/index.html#A15910
Also, this file shows an increased number of violations. We can see that even the old violations now appear on different line numbers, which indicates that the file was modified between the generation of the old and the new reports. so we catched more vioaltions that the new changes in TestPhaseIRMatching.java have introduced

In Old report
Screenshot 2026-02-13 at 11 56 48 PM

In New report
Screenshot 2026-02-13 at 11 57 06 PM

Copy link
Member

@Zopsss Zopsss left a comment

Choose a reason for hiding this comment

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

Really sorry for all the delays, everything looks fine now. Thanks a lot for all the hardwork!!!!

@vivek-0509
Copy link
Contributor Author

@Zopsss Thankyou very much for reviewing

@vivek-0509
Copy link
Contributor Author

@romani PR is waiting for your approval.

@vivek-0509
Copy link
Contributor Author

GitHub, generate report

@vivek-0509
Copy link
Contributor Author

GitHub, generate website

@github-actions
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments