Skip to content

Commit

Permalink
refactor!: Remove tracking of "culprit" from Require methods (deephav…
Browse files Browse the repository at this point in the history 
…en#5739)

The string `culprit` as a marker for pointing to an issue in a stack
trace has never appeared in a DHC issue, nor in DHC slack, and the last
DHE slack message to use it in a mildly helpful way was more than two
years ago.

This patch removes the functionality from `RequirementFailure`, and also
removes any overload of a method in `Require` that would allow callers
to customize the depth within the stack that should be marked.

Partial deephaven#188
Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
  • Loading branch information
@niloc132
niloc132 authored Jul 11, 2024
1 parent 63bcab1 commit 5970ebc
Show file tree
Hide file tree
Showing 8 changed files with 538 additions and 1,755 deletions.
10 changes: 5 additions & 5 deletions Base/src/main/java/io/deephaven/base/stats/Composite.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ public Composite(long now, Value... values) {

// ----------------------------------------------------------------
private static Value[] checkValues(Value[] values) {
Require.neqNull(values, "values", 1);
Require.gtZero(values.length, "values.length", 1);
Require.neqNull(values[0], "values[0]", 1);
Require.neqNull(values, "values");
Require.gtZero(values.length, "values.length");
Require.neqNull(values[0], "values[0]");
char typeTag = values[0].getTypeTag();
for (int nIndex = 1; nIndex < values.length; nIndex++) {
Require.neqNull(values[nIndex], "values[nIndex]", 1);
Require.eq(values[nIndex].getTypeTag(), "values[nIndex].getTypeTag()", typeTag, "typeTag", 1);
Require.neqNull(values[nIndex], "values[nIndex]");
Require.eq(values[nIndex].getTypeTag(), "values[nIndex].getTypeTag()", typeTag, "typeTag");
}
return values;
}
Expand Down
2,118 changes: 513 additions & 1,605 deletions Base/src/main/java/io/deephaven/base/verify/Require.java
View file

Large diffs are not rendered by default.

109 changes: 2 additions & 107 deletions Base/src/main/java/io/deephaven/base/verify/RequirementFailure.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,122 +3,17 @@
//
package io.deephaven.base.verify;

import java.io.PrintStream;
import java.io.PrintWriter;
import java.io.StringWriter;

// ----------------------------------------------------------------
/**
* {@link RuntimeException} to be thrown on requirement failures.
*/
public class RequirementFailure extends RuntimeException {

/**
* The number of stack frames that should be removed from the stack to find the method whose requirements did not
* hold.
*/
private int m_nCallsBelowRequirer;

// ----------------------------------------------------------------
public RequirementFailure(String message, int nCallsBelowRequirer) {
public RequirementFailure(String message) {
super(message);
m_nCallsBelowRequirer = nCallsBelowRequirer;
}

// ----------------------------------------------------------------
public RequirementFailure(String message, Exception caughtException, int nCallsBelowRequirer) {
public RequirementFailure(String message, Exception caughtException) {
super(message, caughtException);
m_nCallsBelowRequirer = nCallsBelowRequirer;
}

// ----------------------------------------------------------------
/**
* Gets the number of stack frames that should be removed from the stack to find the caller which failed to meet
* requirements.
*/
public int getNumCallsBelowRequirer() {
return m_nCallsBelowRequirer;
}

// ----------------------------------------------------------------
@Override
public void printStackTrace() {
printStackTrace(System.err);
}

// ----------------------------------------------------------------
@Override
public void printStackTrace(PrintStream s) {
s.print(getFixedStackTrace());
}

// ----------------------------------------------------------------
@Override
public void printStackTrace(PrintWriter s) {
s.print(getFixedStackTrace());
}

// ----------------------------------------------------------------
/**
* Gets a stack trace with a line added identifying the offending stack frame.
*/
private StringBuffer getFixedStackTrace() {
StringBuffer sb = getOriginalStackTrace();

String sStackTrace = sb.toString();
int nInsertPoint = sStackTrace.indexOf("\n\tat ");
for (int nCount = m_nCallsBelowRequirer; nCount >= 0; nCount--) {
nInsertPoint = sStackTrace.indexOf("\n\tat ", nInsertPoint + 1);
}
if (-1 != nInsertPoint) {
sb.insert(nInsertPoint, "\n (culprit:)");
}
return sb;
}

// ----------------------------------------------------------------
/**
* Gets the unmodified stack trace, instead of the one with the culprit identified.
*/
public StringBuffer getOriginalStackTrace() {
StringWriter stringWriter = new StringWriter();
PrintWriter printWriter = new PrintWriter(stringWriter);
super.printStackTrace(printWriter);
printWriter.close();
return stringWriter.getBuffer();
}

// ----------------------------------------------------------------
/**
* If this stack frame caused the exception, adjust the culprit to be the caller. Used when a delegating method
* can't verify all requirements itself but shouldn't receive the blame.
*/
public RequirementFailure adjustForDelegatingMethod() {
if (isThisStackFrameCulprit(1)) {
m_nCallsBelowRequirer++;
}
return this;
}

// ----------------------------------------------------------------
/**
* If this stack frame caused the exception, adjust the culprit to be the caller. Used when a delegating method
* can't verify all requirements itself but shouldn't receive the blame.
*/
public RequirementFailure adjustForDelegatingMethodAndSyntheticAccessor() {
if (isThisStackFrameCulprit(0)) {
m_nCallsBelowRequirer += 2;
}
return this;
}

// ----------------------------------------------------------------
/**
* Returns true if this stack frame is blamed for causing the exception.
*/
public boolean isThisStackFrameCulprit(int nFramesBelowTargetFrame) {
StackTraceElement[] stackTrace = new Throwable().getStackTrace();
StackTraceElement[] failureStackTrace = getStackTrace();
return failureStackTrace.length - m_nCallsBelowRequirer == stackTrace.length - nFramesBelowTargetFrame;
}
}
3 changes: 1 addition & 2 deletions Base/src/test/java/io/deephaven/base/TestLowGarbageArrayIntegerMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ public void testLowGarbageArrayIntegerMap() {
try {
integerToStringMap.put(-1, "negative one");
fail("expected bad index to fail");
} catch (RequirementFailure requirementFailure) {
assertTrue(requirementFailure.isThisStackFrameCulprit(-1));
} catch (RequirementFailure expected) {
}

try {
Expand Down
12 changes: 6 additions & 6 deletions Base/src/test/java/io/deephaven/base/pool/TestThreadSafeLenientFixedSizePool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ public void testThreadSafeLenientFixedSizePoolNoFactory() {
// too small
try {
new ThreadSafeLenientFixedSizePool<Object>(6, m_mockObjectFactory, m_mockClearingProcedure);
} catch (RequirementFailure requirementFailure) {
assertTrue(requirementFailure.isThisStackFrameCulprit(0));
fail("expected to throw");
} catch (RequirementFailure expected) {
}

// minimum size
Expand All @@ -156,15 +156,15 @@ public void testThreadSafeLenientFixedSizePoolNoFactory() {
// no factory
try {
ThreadSafeLenientFixedSizePool.FACTORY.create(OBJECTS.length, null, m_mockClearingProcedure);
} catch (RequirementFailure requirementFailure) {
assertTrue(requirementFailure.isThisStackFrameCulprit(0));
fail("expected to throw");
} catch (RequirementFailure expected) {
}

// too small
try {
ThreadSafeLenientFixedSizePool.FACTORY.create(6, m_mockObjectFactory, m_mockClearingProcedure);
} catch (RequirementFailure requirementFailure) {
assertTrue(requirementFailure.isThisStackFrameCulprit(0));
fail("expected to throw");
} catch (RequirementFailure expected) {
}

// minimum size
Expand Down
18 changes: 3 additions & 15 deletions DataStructures/src/main/java/io/deephaven/datastructures/util/CollectionUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,29 +30,17 @@ public class CollectionUtil {

// ----------------------------------------------------------------
public static <K, V> Map<K, V> unmodifiableMapFromArray(Class<K> typeK, Class<V> typeV, Object... data) {
try {
return Collections.unmodifiableMap(mapFromArray(typeK, typeV, data));
} catch (RequirementFailure e) {
throw e.adjustForDelegatingMethod();
}
return Collections.unmodifiableMap(mapFromArray(typeK, typeV, data));
}

// ----------------------------------------------------------------
public static <K, V> Map<K, V> unmodifiableInvertMap(Map<V, K> sourceMap) {
try {
return Collections.unmodifiableMap(invertMap(sourceMap));
} catch (RequirementFailure e) {
throw e.adjustForDelegatingMethod();
}
return Collections.unmodifiableMap(invertMap(sourceMap));
}

// ----------------------------------------------------------------
public static <E> Set<E> unmodifiableSetFromArray(E... data) {
try {
return Collections.unmodifiableSet(setFromArray(data));
} catch (RequirementFailure e) {
throw e.adjustForDelegatingMethod();
}
return Collections.unmodifiableSet(setFromArray(data));
}

// ----------------------------------------------------------------
Expand Down
11 changes: 2 additions & 9 deletions Util/src/main/java/io/deephaven/util/Utils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,19 +375,12 @@ public static String getSimpleNameFor(@NotNull Class<?> objectClass) {
/**
* require (o instanceof type)
*/
public static <T> T castTo(Object o, String name, Class<T> type, int numCallsBelowRequirer) {
io.deephaven.base.verify.Require.instanceOf(o, name, type, numCallsBelowRequirer);
public static <T> T castTo(Object o, String name, Class<T> type) {
io.deephaven.base.verify.Require.instanceOf(o, name, type);
// noinspection unchecked
return (T) o;
}

/**
* require (o instanceof type)
*/
public static <T> T castTo(Object o, String name, Class<T> type) {
return castTo(o, name, type, 1);
}

/**
* Describe the object in a standardized format without accessing its fields or otherwise risking interacting with
* partially-initialized state.
Expand Down
12 changes: 6 additions & 6 deletions Util/src/test/java/io/deephaven/util/pool/TestThreadSafeLenientFixedSizePool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@ public void testThreadSafeLenientFixedSizePoolNoFactory() {
try {
new io.deephaven.base.pool.ThreadSafeLenientFixedSizePool<Object>(6, m_mockObjectFactory,
m_mockClearingProcedure);
} catch (RequirementFailure requirementFailure) {
assertTrue(requirementFailure.isThisStackFrameCulprit(0));
fail("expected to throw");
} catch (RequirementFailure expected) {
}

// minimum size
Expand All @@ -162,16 +162,16 @@ public void testThreadSafeLenientFixedSizePoolNoFactory() {
try {
io.deephaven.base.pool.ThreadSafeLenientFixedSizePool.FACTORY.create(OBJECTS.length, null,
m_mockClearingProcedure);
} catch (RequirementFailure requirementFailure) {
assertTrue(requirementFailure.isThisStackFrameCulprit(0));
fail("expected to throw");
} catch (RequirementFailure expected) {
}

// too small
try {
io.deephaven.base.pool.ThreadSafeLenientFixedSizePool.FACTORY.create(6, m_mockObjectFactory,
m_mockClearingProcedure);
} catch (RequirementFailure requirementFailure) {
assertTrue(requirementFailure.isThisStackFrameCulprit(0));
fail("expected to throw");
} catch (RequirementFailure expected) {
}

// minimum size
Expand Down

0 comments on commit 5970ebc

Please sign in to comment.