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

Fix for false positives EI_EXPOSE_REP in case of unmodifiable collections #2141

Merged
merged 4 commits into from Sep 1, 2022

Conversation

baloghadamsoftware
Copy link
Contributor

The methods of and copyOf of List, Map and Set return an unmodifiable collection. Thus if final fields are initiablized using these methods then returning them is not dangerous.


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

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

…ctions

The methods `of` and `copyOf` of `List`, `Map` and `Set` return an unmodifiable collection. Thus if final fields are initiablized using these methods then returning them is not dangerous.
CHANGELOG.md Outdated Show resolved Hide resolved
ThrawnCA
ThrawnCA previously approved these changes Sep 1, 2022
KengoTODA
KengoTODA previously approved these changes Sep 1, 2022
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@KengoTODA KengoTODA dismissed stale reviews from ThrawnCA and themself via 98fbc8d September 1, 2022 21:24
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

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

I just resolved a conflict in the CHANGELOG, so I treat this PR as approved by two SpotBugs teammates.

@trancexpress
Copy link
Contributor

This causes a regression, see: #2174

@zbynek
Copy link

zbynek commented Sep 12, 2022

Also since this change I get hundreds of lines of spam in the console (running on Java 8):

Exception analyzing org.geogebra.desktop.main.MyResourceBundle using detector edu.umd.cs.findbugs.detect.LoadOfKnownNullValue
    org.apache.bcel.classfile.ClassFormatException: Invalid signature: Ljava/util/Collections$UnmodifiableRandomAccessList
      At org.apache.bcel.classfile.Utility.typeSignatureToString(Utility.java:997)
      At org.apache.bcel.generic.Type.getType(Type.java:215)
      At edu.umd.cs.findbugs.ba.type.TypeFrameModelingVisitor.getType(TypeFrameModelingVisitor.java:399)
      At edu.umd.cs.findbugs.ba.type.TypeFrameModelingVisitor.modelFieldLoad(TypeFrameModelingVisitor.java:361)
      At edu.umd.cs.findbugs.ba.type.TypeFrameModelingVisitor.visitGETSTATIC(TypeFrameModelingVisitor.java:346)
      At org.apache.bcel.generic.GETSTATIC.accept(GETSTATIC.java:76)
      At edu.umd.cs.findbugs.ba.AbstractFrameModelingVisitor.analyzeInstruction(AbstractFrameModelingVisitor.java:84)
      At edu.umd.cs.findbugs.ba.type.TypeFrameModelingVisitor.analyzeInstruction(TypeFrameModelingVisitor.java:196)
      At edu.umd.cs.findbugs.ba.type.TypeAnalysis.transferInstruction(TypeAnalysis.java:406)
      At edu.umd.cs.findbugs.ba.type.TypeAnalysis.transferInstruction(TypeAnalysis.java:86)
      At edu.umd.cs.findbugs.ba.AbstractDataflowAnalysis.transfer(AbstractDataflowAnalysis.java:136)
      At edu.umd.cs.findbugs.ba.type.TypeAnalysis.transfer(TypeAnalysis.java:414)
      At edu.umd.cs.findbugs.ba.type.TypeAnalysis.transfer(TypeAnalysis.java:86)
      At edu.umd.cs.findbugs.ba.Dataflow.execute(Dataflow.java:378)
      At edu.umd.cs.findbugs.classfile.engine.bcel.TypeDataflowFactory.analyze(TypeDataflowFactory.java:83)
      At edu.umd.cs.findbugs.classfile.engine.bcel.TypeDataflowFactory.analyze(TypeDataflowFactory.java:43)
      At edu.umd.cs.findbugs.classfile.impl.AnalysisCache.analyzeMethod(AnalysisCache.java:368)
      At edu.umd.cs.findbugs.classfile.impl.AnalysisCache.getMethodAnalysis(AnalysisCache.java:321)
      At edu.umd.cs.findbugs.classfile.engine.bcel.CFGFactory.analyze(CFGFactory.java:160)
      At edu.umd.cs.findbugs.classfile.engine.bcel.CFGFactory.analyze(CFGFactory.java:65)
      At edu.umd.cs.findbugs.classfile.impl.AnalysisCache.analyzeMethod(AnalysisCache.java:368)
      At edu.umd.cs.findbugs.classfile.impl.AnalysisCache.getMethodAnalysis(AnalysisCache.java:321)
      At edu.umd.cs.findbugs.ba.ClassContext.getMethodAnalysis(ClassContext.java:1010)
      At edu.umd.cs.findbugs.ba.ClassContext.getMethodAnalysisNoDataflowAnalysisException(ClassContext.java:995)
      At edu.umd.cs.findbugs.ba.ClassContext.getCFG(ClassContext.java:301)
      At edu.umd.cs.findbugs.detect.FindUseOfNonSerializableValue.analyzeMethod(FindUseOfNonSerializableValue.java:143)
      At edu.umd.cs.findbugs.detect.FindUseOfNonSerializableValue.visitClassContext(FindUseOfNonSerializableValue.java:95)
      At edu.umd.cs.findbugs.DetectorToDetector2Adapter.visitClass(DetectorToDetector2Adapter.java:76)
      At edu.umd.cs.findbugs.FindBugs2.lambda$analyzeApplication$1(FindBugs2.java:1108)
      At java.util.concurrent.FutureTask.run(FutureTask.java:266)
      At edu.umd.cs.findbugs.CurrentThreadExecutorService.execute(CurrentThreadExecutorService.java:86)
      At java.util.concurrent.AbstractExecutorService.invokeAll(AbstractExecutorService.java:238)
      At edu.umd.cs.findbugs.FindBugs2.analyzeApplication(FindBugs2.java:1118)
      At edu.umd.cs.findbugs.FindBugs2.execute(FindBugs2.java:309)
      At edu.umd.cs.findbugs.FindBugs.runMain(FindBugs.java:395)
      At edu.umd.cs.findbugs.FindBugs2.main(FindBugs2.java:1231)

Does this require Java 11?

@iloveeclipse
Copy link
Member

Does this require Java 11?

I saw this on Java 17 too.

@baloghadamsoftware
Copy link
Contributor Author

I will check this. Actually, I tested the PR on several open-source projects and instead of regressions it reduced the number of false positives by a lot. First, we need to reproduce it somehow on a small example.

@baloghadamsoftware
Copy link
Contributor Author

Invalid signature: Ljava/util/Collections$UnmodifiableRandomAccessList <- Here, I see that a semicolon is missing from the signature. I will try to create a test case that fails on this before fixing this issue.

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

6 participants