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.

Sign up for GitHub

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

Jump to bottom

Enhancement for issue #2061. #2069

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+88 −64
Open

Enhancement for issue #2061. #2069

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0eef2c
Enhancement for issue #2061.
Sustech-yx May 25, 2022
5062959
Modify the enum name.
Sustech-yx May 25, 2022
e21c708
Fix build error.
Sustech-yx May 31, 2022
637eb64
Merge branch 'master' into issue#2061
hazendaz Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -5,6 +5,9 @@ This is the changelog for SpotBugs. This follows [Keep a Changelog v1.0.0](http:
Currently the versioning policy of this project follows [Semantic Versioning v2.0.0](http://semver.org/spec/v2.0.0.html).

## Unreleased - 2022-??-??
### Changed
- Rewrite some member in `ResourceValueFrame.java` to Enum([#2061](https://github.com/spotbugs/spotbugs/issues/2061))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a major, breaking change, IMO it would be better, if the changelog contained that it's a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Is it though? Is there external usage of this? I don't see it in the gradle or maven plugins, maybe something else is using it. Can call it breaking I guess given its public got thinking its ok.


### Fixed
- Fixed False positives for `RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE` on try-with-resources with interface references ([#1931](https://github.com/spotbugs/spotbugs/issues/1931))

Expand Down
10 changes: 5 additions & 5 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueAnalysis.java
View file
Open in desktop
Expand Up @@ -102,10 +102,10 @@ public void meetInto(ResourceValueFrame fact, Edge edge, ResourceValueFrame resu
return;
}

if (fact.getStatus() == ResourceValueFrame.OPEN) {
if (fact.getStatus() == ResourceValueFrame.State.OPEN) {
// If status is OPEN, downgrade to OPEN_ON_EXCEPTION_PATH
tmpFact = modifyFrame(fact, null);
tmpFact.setStatus(ResourceValueFrame.OPEN_ON_EXCEPTION_PATH);
tmpFact.setStatus(ResourceValueFrame.State.OPEN_ON_EXCEPTION_PATH);
}

if (fact.isValid()) {
Expand All @@ -122,7 +122,7 @@ public void meetInto(ResourceValueFrame fact, Edge edge, ResourceValueFrame resu
&& resourceTracker.isResourceClose(fallThroughSuccessor, exceptionThrower, methodGen.getConstantPool(),
resource, fact)) {
tmpFact = modifyFrame(fact, tmpFact);
tmpFact.setStatus(ResourceValueFrame.CLOSED);
tmpFact.setStatus(ResourceValueFrame.State.CLOSED);
if (DEBUG) {
System.out.print("(failed attempt to close)");
}
Expand Down Expand Up @@ -192,7 +192,7 @@ else if (lastInSource instanceof IFNULL || lastInSource instanceof IFNONNULL) {
if ((isNullCheck && edgeType == IFCMP_EDGE) || (isNonNullCheck && edgeType == FALL_THROUGH_EDGE)) {
// System.out.println("**** making resource nonexistent on edge "+edge.getId());
tmpFact = modifyFrame(fact, tmpFact);
tmpFact.setStatus(ResourceValueFrame.NONEXISTENT);
tmpFact.setStatus(ResourceValueFrame.State.NONEXISTENT);
}
}
}
Expand All @@ -213,7 +213,7 @@ protected void mergeInto(ResourceValueFrame frame, ResourceValueFrame result) th
super.mergeInto(frame, result);

// Merge status
result.setStatus(Math.min(result.getStatus(), frame.getStatus()));
result.setStatus(result.getStatus().getType() > frame.getStatus().getType() ? frame.getStatus() : result.getStatus());
}

@Override
Expand Down
87 changes: 50 additions & 37 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueFrame.java
View file
Open in desktop
Expand Up @@ -20,50 +20,63 @@
package edu.umd.cs.findbugs.ba;

public class ResourceValueFrame extends Frame<ResourceValue> {
/**
* The resource escapes the method.
*/
public static final int ESCAPED = 0;

/**
* The resource is open (or locked, etc) on paths that include only normal
* control flow.
*/
public static final int OPEN = 1;

/**
* The resource is open (or locked, etc) on paths that include exception
* control flow.
*/
public static final int OPEN_ON_EXCEPTION_PATH = 2;

/**
* The resource is closed (or unlocked, etc).
*/
public static final int CLOSED = 3;

/**
* The resource has been created, but is not open.
*/
public static final int CREATED = 4;

/**
* The resource doesn't exist.
*/
public static final int NONEXISTENT = 5;

private int status;

public enum State {
/**
* The resource escapes the method.
*/
ESCAPED(0),

/**
* The resource is open (or locked, etc) on paths that include only normal
* control flow.
*/
OPEN(1),

/**
* The resource is open (or locked, etc) on paths that include exception
* control flow.
*/
OPEN_ON_EXCEPTION_PATH(2),

/**
* The resource is closed (or unlocked, etc).
*/
CLOSED(3),

/**
* The resource has been created, but is not open.
*/
CREATED(4),

/**
* The resource doesn't exist.
*/
NONEXISTENT(5);

State(int type) {
this.type = type;
}

private final int type;

public int getType() {
return type;
}
}

private State status;

public ResourceValueFrame(int numSlots) {
super(numSlots);
this.status = NONEXISTENT;
this.status = State.NONEXISTENT;
}

public int getStatus() {
public State getStatus() {
return status;
}

public void setStatus(int status) {
public void setStatus(State status) {
this.status = status;
}

Expand All @@ -89,7 +102,7 @@ public void copyFrom(Frame<ResourceValue> other_) {

@Override
public String toString() {
return super.toString() + statusList[status];
return super.toString() + statusList[status.getType()];
}

}
10 changes: 5 additions & 5 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueFrameModelingVisitor.java
View file
Open in desktop
Expand Up @@ -63,7 +63,7 @@ private void handleFieldStore(FieldInstruction ins) {
ResourceValueFrame frame = getFrame();
ResourceValue topValue = frame.getTopValue();
if (topValue.equals(ResourceValue.instance())) {
frame.setStatus(ResourceValueFrame.ESCAPED);
frame.setStatus(ResourceValueFrame.State.ESCAPED);
}
} catch (DataflowAnalysisException e) {
throw new InvalidBytecodeException("Stack underflow", e);
Expand All @@ -86,7 +86,7 @@ private void handleArrayStore(ArrayInstruction ins) {
ResourceValueFrame frame = getFrame();
ResourceValue topValue = frame.getTopValue();
if (topValue.equals(ResourceValue.instance())) {
frame.setStatus(ResourceValueFrame.ESCAPED);
frame.setStatus(ResourceValueFrame.State.ESCAPED);
}
} catch (DataflowAnalysisException e) {
throw new InvalidBytecodeException("Stack underflow", e);
Expand Down Expand Up @@ -135,7 +135,7 @@ private void handleInvoke(InvokeInstruction inv) {
}

if (instanceArgNum >= 0 && instanceEscapes(inv, instanceArgNum)) {
frame.setStatus(ResourceValueFrame.ESCAPED);
frame.setStatus(ResourceValueFrame.State.ESCAPED);
}

handleNormalInstruction(inv);
Expand All @@ -150,7 +150,7 @@ public void visitCHECKCAST(CHECKCAST obj) {
topValue = frame.getTopValue();

if (topValue.equals(ResourceValue.instance())) {
frame.setStatus(ResourceValueFrame.ESCAPED);
frame.setStatus(ResourceValueFrame.State.ESCAPED);
}
} catch (DataflowAnalysisException e) {
AnalysisContext.logError("Analysis error", e);
Expand Down Expand Up @@ -183,7 +183,7 @@ public void visitARETURN(ARETURN ins) {
ResourceValueFrame frame = getFrame();
ResourceValue topValue = frame.getTopValue();
if (topValue.equals(ResourceValue.instance())) {
frame.setStatus(ResourceValueFrame.ESCAPED);
frame.setStatus(ResourceValueFrame.State.ESCAPED);
}
} catch (DataflowAnalysisException e) {
throw new InvalidBytecodeException("Stack underflow", e);
Expand Down
9 changes: 5 additions & 4 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOpenStream.java
View file
Open in desktop
Expand Up @@ -488,21 +488,22 @@ public void inspectResult(ClassContext classContext, MethodGen methodGen, CFG cf
}
ResourceValueFrame exitFrame = dataflow.getResultFact(cfg.getExit());

int exitStatus = exitFrame.getStatus();
if (exitStatus == ResourceValueFrame.OPEN || exitStatus == ResourceValueFrame.OPEN_ON_EXCEPTION_PATH) {
ResourceValueFrame.State exitStatus = exitFrame.getStatus();
if (exitStatus == ResourceValueFrame.State.OPEN ||
exitStatus == ResourceValueFrame.State.OPEN_ON_EXCEPTION_PATH) {

// FIXME: Stream object should be queried for the
// priority.

String bugType = stream.getBugType();
int priority = NORMAL_PRIORITY;
if (exitStatus == ResourceValueFrame.OPEN_ON_EXCEPTION_PATH) {
if (exitStatus == ResourceValueFrame.State.OPEN_ON_EXCEPTION_PATH) {
bugType += "_EXCEPTION_PATH";
priority = LOW_PRIORITY;
}

potentialOpenStreamList.add(new PotentialOpenStream(bugType, priority, stream));
} else if (exitStatus == ResourceValueFrame.CLOSED) {
} else if (exitStatus == ResourceValueFrame.State.CLOSED) {
// Remember that this stream was closed on all paths.
// Later, we will mark all of the streams in its equivalence class
// as having been closed.
Expand Down
18 changes: 11 additions & 7 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindUnreleasedLock.java
View file
Open in desktop
Expand Up @@ -108,7 +108,8 @@ public void transferInstruction(InstructionHandle handle, BasicBlock basicBlock)
final ConstantPoolGen cpg = getCPG();
final ResourceValueFrame frame = getFrame();

int status = -1;
ResourceValueFrame.State status = ResourceValueFrame.State.NONEXISTENT;
boolean updated = false;

if (DEBUG) {
System.out.println("PC : " + handle.getPosition() + " " + ins);
Expand All @@ -123,12 +124,14 @@ public void transferInstruction(InstructionHandle handle, BasicBlock basicBlock)
// Is a lock acquired or released by this instruction?
Location creationPoint = lock.getLocation();
if (handle == creationPoint.getHandle() && basicBlock == creationPoint.getBasicBlock()) {
status = ResourceValueFrame.OPEN;
status = ResourceValueFrame.State.OPEN;
updated = true;
if (DEBUG) {
System.out.println("OPEN");
}
} else if (resourceTracker.isResourceClose(basicBlock, handle, cpg, lock, frame)) {
status = ResourceValueFrame.CLOSED;
status = ResourceValueFrame.State.CLOSED;
updated = true;
if (DEBUG) {
System.out.println("CLOSE");
}
Expand Down Expand Up @@ -166,7 +169,7 @@ public void transferInstruction(InstructionHandle handle, BasicBlock basicBlock)
}

// If needed, update frame status
if (status != -1) {
if (updated) {
frame.setStatus(status);
}
if (DEBUG) {
Expand Down Expand Up @@ -431,12 +434,13 @@ public void inspectResult(ClassContext classContext, MethodGen methodGen, CFG cf
if (DEBUG) {
System.out.println("Resource value at exit: " + exitFrame);
}
int exitStatus = exitFrame.getStatus();
ResourceValueFrame.State exitStatus = exitFrame.getStatus();

if (exitStatus == ResourceValueFrame.OPEN || exitStatus == ResourceValueFrame.OPEN_ON_EXCEPTION_PATH) {
if (exitStatus == ResourceValueFrame.State.OPEN ||
exitStatus == ResourceValueFrame.State.OPEN_ON_EXCEPTION_PATH) {
String bugType;
int priority;
if (exitStatus == ResourceValueFrame.OPEN) {
if (exitStatus == ResourceValueFrame.State.OPEN) {
bugType = "UL_UNRELEASED_LOCK";
priority = HIGH_PRIORITY;
} else {
Expand Down
16 changes: 10 additions & 6 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/detect/StreamFrameModelingVisitor.java
View file
Open in desktop
Expand Up @@ -57,36 +57,40 @@ public void transferInstruction(InstructionHandle handle, BasicBlock basicBlock)
final Instruction ins = handle.getInstruction();
final ResourceValueFrame frame = getFrame();

int status = -1;
ResourceValueFrame.State status = ResourceValueFrame.State.NONEXISTENT;
boolean updated = false;
boolean created = false;

// Is a resource created, opened, or closed by this instruction?
Location creationPoint = stream.getLocation();
if (handle == creationPoint.getHandle() && basicBlock == creationPoint.getBasicBlock()) {
// Resource creation
if (stream.isOpenOnCreation()) {
status = ResourceValueFrame.OPEN;
status = ResourceValueFrame.State.OPEN;
stream.setOpenLocation(location);
resourceTracker.addStreamOpenLocation(location, stream);
} else {
status = ResourceValueFrame.CREATED;
status = ResourceValueFrame.State.CREATED;
}
updated = true;
created = true;
} else if (resourceTracker.isResourceOpen(basicBlock, handle, cpg, stream, frame)) {
// Resource opened
status = ResourceValueFrame.OPEN;
status = ResourceValueFrame.State.OPEN;
updated = true;
stream.setOpenLocation(location);
resourceTracker.addStreamOpenLocation(location, stream);
} else if (resourceTracker.isResourceClose(basicBlock, handle, cpg, stream, frame)) {
// Resource closed
status = ResourceValueFrame.CLOSED;
status = ResourceValueFrame.State.CLOSED;
updated = true;
}

// Model use of instance values in frame slots
analyzeInstruction(ins);

// If needed, update frame status
if (status != -1) {
if (updated) {
frame.setStatus(status);
if (created) {
frame.setValue(frame.getNumSlots() - 1, ResourceValue.instance());
Expand Down