Skip to content

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

Merged
KengoTODA merged 4 commits intospotbugs:masterfrom
baloghadamsoftware:Issue1771
Sep 1, 2022
Merged

Fix for false positives EI_EXPOSE_REP in case of unmodifiable collections#2141
KengoTODA merged 4 commits intospotbugs:masterfrom
baloghadamsoftware:Issue1771

Conversation

@baloghadamsoftware
Copy link
Copy Markdown
Contributor

@baloghadamsoftware baloghadamsoftware commented Aug 13, 2022

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

Fix for false positives EI_EXPOSE_REP in case of unmodifiable colle…
9303a7e 
…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.
@baloghadamsoftware baloghadamsoftware mentioned this pull request Aug 13, 2022
Open
ThrawnCA
ThrawnCA requested changes Aug 14, 2022
View reviewed changes
Comment thread CHANGELOG.md Outdated
Comment thread spotbugs/src/main/java/edu/umd/cs/findbugs/OpcodeStack.java Outdated
@baloghadamsoftware baloghadamsoftware mentioned this pull request Aug 30, 2022
Open
ThrawnCA
ThrawnCA previously approved these changes Sep 1, 2022
View reviewed changes
KengoTODA
KengoTODA previously approved these changes Sep 1, 2022
View reviewed changes
Copy link
Copy Markdown
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
KengoTODA
KengoTODA approved these changes Sep 1, 2022
View reviewed changes
Copy link
Copy Markdown
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.

@KengoTODA KengoTODA merged commit 4c9b635 into spotbugs:master Sep 1, 2022
@baloghadamsoftware baloghadamsoftware mentioned this pull request Sep 9, 2022
Closed
@trancexpress
Copy link
Copy Markdown
Contributor

trancexpress commented Sep 12, 2022

This causes a regression, see: #2174

@zbynek
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

iloveeclipse commented Sep 12, 2022

Does this require Java 11?

I saw this on Java 17 too.

@baloghadamsoftware
Copy link
Copy Markdown
Contributor Author

baloghadamsoftware commented Sep 12, 2022

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
Copy Markdown
Contributor Author

baloghadamsoftware commented Sep 12, 2022

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.

@gtoison gtoison mentioned this pull request Sep 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KengoTODA KengoTODA KengoTODA approved these changes

@ThrawnCA ThrawnCA ThrawnCA left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@baloghadamsoftware @trancexpress @zbynek @iloveeclipse @KengoTODA @ThrawnCA