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.

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

DCL01-J. Do not reuse public identifiers from the Java Standard Library #2511

Merged
merged 71 commits into from
Sep 8, 2023

Conversation

norbo03
Copy link
Contributor

@norbo03 norbo03 commented Aug 14, 2023

I created DontReusePublicIdentifiers detector for rule DCL01-J to check clashes between user-defined identifiers and publicly available identifiers from the Java Standard Library.

I created several test cases considering the possible locations where user-defined names would appear. I checked class and interface names, field and variable declarations, and method names. In the case of classes, I checked multiply nested classes too. The test classes are placed into a separate package because of the number of source files.

I have slightly adjusted AnnotationMatcherTest because my detector found bugs in the test classes and the test was failing. Instead of checking that found bug matches the annotation, it inspects the number of bugs.


Make sure these boxes are checked before submitting your PR -- thank you!

  • Added an entry into CHANGELOG.md if you have changed SpotBugs code

Restructure test cases for class names into separate packages, representing where the bug should appear.
Add new edge cases.

[Ticket: spotbugs#32]
Separated test cases into individual methods for improved clarity and modularity.

[Ticket: spotbugs#32]
Rename test classes in order to be more descriptive

[Ticket: spotbugs#32]
Implement test cases for shadowing with fields and variables. Also added the necessary test classes

[Ticket: spotbugs#32]
Add test case with static method and add another test case with method parameter

[Ticket: spotbugs#32]
[Ticket: java-static-analysis/dev-spotbugs#32]
Since there are so many public identifiers, they are ordered alphabetically and split into separate files, roughly containing the same amount of identifiers.
It is necessary to split in order to avoid compiler limitations.

[Ticket: java-static-analysis/dev-spotbugs#32]
[Ticket: java-static-analysis/dev-spotbugs#32]
[Ticket: java-static-analysis/dev-spotbugs#32]
[Ticket: java-static-analysis/dev-spotbugs#32]
[Ticket: java-static-analysis/dev-spotbugs#32]
[Ticket: java-static-analysis/dev-spotbugs#32]
Remove unnecessary new lines
Remove redundant null value initialization

[Ticket: java-static-analysis/dev-spotbugs#32]
AnnotationMatcherTest uses two public identifiers from the Java Standard Library (namely 'Value' and 'Generated') that are found by DontReusePublicIdentifiers detector.
Fix: checking for the public identifier bugtype and ignoring those cases

[Ticket: java-static-analysis/dev-spotbugs#32]
[Ticket: java-static-analysis/dev-spotbugs#32]
[Ticket: java-static-analysis/dev-spotbugs#32]
[Ticket: java-static-analysis/dev-spotbugs#32]
Ticket: java-static-analysis/dev-spotbugs#32
Copy link
Contributor

@ThrawnCA ThrawnCA left a comment

Choose a reason for hiding this comment

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

Build is failing.

Copy link
Collaborator

@JuditKnoll JuditKnoll left a comment

Choose a reason for hiding this comment

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

It looks like there are some merge problems. Can you please resolve them?

CHANGELOG.md Outdated Show resolved Hide resolved
spotbugs-tests/src/test/resources/firstFile.xml Outdated Show resolved Hide resolved
spotbugs-tests/src/test/resources/secondFile.xml Outdated Show resolved Hide resolved
@JuditKnoll
Copy link
Collaborator

Build is failing.

I think earlier "only" the Read the docs build failed, not the actual spotbugs build. Approved the workflow, the build succeeded. @ThrawnCA Do you know how to prevent the Read the docs build from failing?

@JuditKnoll
Copy link
Collaborator

Build is failing.

I think earlier "only" the Read the docs build failed, not the actual spotbugs build. Approved the workflow, the build succeeded. @ThrawnCA Do you know how to prevent the Read the docs build from failing?

The Read the docs build problem got solved.
@norbo03 Can you please merge master to your branch?

@gtoison
Copy link
Contributor

gtoison commented Aug 22, 2023

Thanks for the PR @norbo03
I looked at the list of public identifiers and I think it should only include the public parts of the JDK. For instance the Axis class name is twice (!) in the JDK, in com.sun.org.apache.xerces.internal.impl.xpath.XPath.Axis and com.sun.org.apache.xml.internal.dtm.XPath. Neither of these classes in "exported" packages according to the module-info of these modules, so basically they shouldn't be used at all, and I'd argue that naming your own class "Axis" would be fine.

I suppose that this identifiers list was built with some kind of script, would it be possible to share the script?
Maybe this might be part of the project build, so new classes added to the JDK get added automatically as we upgrade versions?
There are also some .db in the project for this kind of long list, for instance https://github.com/spotbugs/spotbugs/blob/master/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/longInstant.db

@norbo03
Copy link
Contributor Author

norbo03 commented Aug 26, 2023

Thanks for the PR @norbo03 I looked at the list of public identifiers and I think it should only include the public parts of the JDK. For instance the Axis class name is twice (!) in the JDK, in com.sun.org.apache.xerces.internal.impl.xpath.XPath.Axis and com.sun.org.apache.xml.internal.dtm.XPath. Neither of these classes in "exported" packages according to the module-info of these modules, so basically they shouldn't be used at all, and I'd argue that naming your own class "Axis" would be fine.

I suppose that this identifiers list was built with some kind of script, would it be possible to share the script? Maybe this might be part of the project build, so new classes added to the JDK get added automatically as we upgrade versions? There are also some .db in the project for this kind of long list, for instance https://github.com/spotbugs/spotbugs/blob/master/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/longInstant.db

I believe integrating an automatic update feature into the project is a fantastic idea! In the past, I utilized scripts to extract public identifiers. Regrettably, these scripts are now outdated, and the results they produced necessitated extensive manual adjustments. While these scripts could serve as a foundation, they do require updates. I've gathered them here for reference.

@gtoison
Copy link
Contributor

gtoison commented Aug 27, 2023

Thanks, I can't seem to access the repository you have linked (I get a 404 error page), maybe it is private?

@norbo03
Copy link
Contributor Author

norbo03 commented Aug 27, 2023

Thanks, I can't seem to access the repository you have linked (I get a 404 error page), maybe it is private?

I have already changed it, can you please check it now?

@hazendaz hazendaz self-assigned this Sep 8, 2023
@hazendaz hazendaz dismissed JuditKnoll’s stale review September 8, 2023 00:15

merge conflicts were resolved.

@hazendaz hazendaz merged commit cfc3597 into spotbugs:master Sep 8, 2023
6 checks passed
@hazendaz hazendaz added this to the SpotBugs 4.8.0 milestone Dec 18, 2023
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.

None yet

5 participants