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

feat: Add CtTry.addCatcherAt #4954

Merged
merged 8 commits into from
Oct 23, 2022
Merged

feat: Add CtTry.addCatcherAt #4954

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b7492a6
Add method to add catcher at a specified position in the try statement
algomaster99 Oct 12, 2022
3109fa1
Add tests
algomaster99 Oct 12, 2022
8bc0965
Add Javadoc
algomaster99 Oct 12, 2022
44f4d89
Improve Javadoc
algomaster99 Oct 12, 2022
0510d3e
Update src/test/java/spoon/test/trycatch/TryCatchTest.java
algomaster99 Oct 13, 2022
3b7bd25
Update src/test/java/spoon/test/trycatch/TryCatchTest.java
algomaster99 Oct 13, 2022
1401c80
Incorporate changes
algomaster99 Oct 13, 2022
0a4c803
Give co-authorship to @slarse
algomaster99 Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/main/java/spoon/reflect/code/CtTry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,19 @@ public interface CtTry extends CtStatement, TemplateParameter<Void>, CtBodyHolde
@PropertySetter(role = CATCH)
<T extends CtTry> T addCatcher(CtCatch catcher);

/**
* Adds a catch block at the specified position in the <code>try</code> statement.
* Behaves similarly to {@link java.util.List#add(int, Object)}.
*
* @param <T> the type of the try statement
* @param position the position at which the <code>catcher</code> is to be inserted
* @param catcher the catch statement to be inserted
* @return this try statement
* @throws IndexOutOfBoundsException if the position is out of range (position < 0 || position > number of catchers)
*/
@PropertySetter(role = CATCH)
<T extends CtTry> T addCatcherAt(int position, CtCatch catcher);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using position to refer to index in the catchers collection because it has been used for all analogous methods. See

spoon/src/main/java/spoon/reflect/code/CtAbstractSwitch.java

Line 72 in 76ffd33

<T extends CtAbstractSwitch<S>> T addCaseAt(int position, CtCase<? super S> c);
as an example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a fine enough word. "Index" is rather data structure specific, position feels more implementation agnostic to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<T extends CtTry> T addCatcherAt(int position, CtCatch catcher);
CtTry addCatcherAt(int position, CtCatch catcher);

This is a design flaw that we need to, well, not do anymore :).

I haven't been around for long enough to know why this is so prevalent in Spoon, but it's a terrible misuse of Java's generics. To illustrate why a return type like this is so problematic, consider the following code:

CtTry tryStatement = new CtTryImpl();
CtCatch catcher = new CtCatchImpl();
CtTryWithResource tryWithResource = tryStatement.addCatcherAt(0, catcher);

This will compile just fine without even a warning, but result in the following runtime error:

java.lang.ClassCastException: class spoon.support.reflect.code.CtTryImpl cannot be cast to class spoon.reflect.code.CtTryWithResource (spoon.support.reflect.code.CtTryImpl and spoon.reflect.code.CtTryWithResource are in unnamed module of loader 'app')

That's incredibly confusing as the calling code hasn't performed any casts, much less any unsafe casts.

One could argue that we should keep using this pattern for the interface to be consistent, but this pattern is so incredibly problematic that I think breaking consistency is a small price to pay for not making this problem any larger than it already is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is indeed confusing, but why does it throw the class cast exception? CtTryWithResource extends from CtTry so it should be valid during runtime as well. Is it because type erasure replaces <T extends CtTry> to CtTry? I do not understand what exactly is happening at runtime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

CtTryWithResource extends from CtTry so it should be valid during runtime as well.

Yes, but the actual dynamic type of the tryStatement variable is CtTryImpl, which does not implement CtTryWithResource. So, there's an exception when the runtime tries to cast to the latter.

I wrote a really elaborate explanation for this, and then the page refreshed and I lost it :D. I'll give it a go again tomorrow, but the very short explanation is that the problems occur with the unchecked cast in the return statement (i.e. (T)). Basically, it's not a cast at all, it's just telling the compiler "trust me mate, this is fine".

The actual cast here occurs when we assign to the variable with type CtTryWithResource after the method has returned a value. That's where the crash occurs. Java is hailed as a statically typed language, but it's actually very much a dynamically typed language as well, and implicit type checks occur all the time wherever generics are involved.

I'll try to circle back around to the rather nasty ramifications this stuff can have: a confusing ClassCastException is just the tip of the iceberg.

I spent way too much time on writing (and losing) the explanation here so I'll have to get back to the actual PR at a later time :)


/**
* Removes a catch block.
*/
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/spoon/support/reflect/code/CtTryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ public <T extends CtTry> T setCatchers(List<CtCatch> catchers) {

@Override
public <T extends CtTry> T addCatcher(CtCatch catcher) {
addCatcherAt(catchers.size(), catcher);
return (T) this;
}

@Override
public <T extends CtTry> T addCatcherAt(int position, CtCatch catcher) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public <T extends CtTry> T addCatcherAt(int position, CtCatch catcher) {
public CtTry T addCatcherAt(int position, CtCatch catcher) {

Same comment as on the interface.

if (catcher == null) {
return (T) this;
Copy link
Contributor

Choose a reason for hiding this comment

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

💬 2 similar findings have been found in this PR

unchecked: unchecked cast

🔎 Expand here to view all instances of this finding
File Path Line Number
src/main/java/spoon/support/reflect/code/CtTryImpl.java 68
src/main/java/spoon/support/reflect/code/CtTryImpl.java 82

Visit the Lift Web Console to find more details in your report.

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

}
Expand All @@ -72,7 +78,7 @@ public <T extends CtTry> T addCatcher(CtCatch catcher) {
}
catcher.setParent(this);
getFactory().getEnvironment().getModelChangeListener().onListAdd(this, CATCH, this.catchers, catcher);
catchers.add(catcher);
catchers.add(position, catcher);
return (T) this;
}

Expand Down
60 changes: 59 additions & 1 deletion src/test/java/spoon/test/trycatch/TryCatchTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.Set;

import org.apache.commons.lang3.StringUtils;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import spoon.Launcher;
Expand All @@ -36,7 +37,6 @@
import spoon.reflect.code.CtResource;
import spoon.reflect.code.CtTry;
import spoon.reflect.code.CtTryWithResource;
import spoon.reflect.code.CtVariableRead;
import spoon.reflect.declaration.CtClass;
import spoon.reflect.declaration.CtMethod;
import spoon.reflect.declaration.CtVariable;
Expand All @@ -56,11 +56,13 @@

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.collection.IsIterableContainingInOrder.contains;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static spoon.testing.utils.ModelUtils.build;
Expand Down Expand Up @@ -442,4 +444,60 @@ public void testTryWithVariableAsResource() {
tryStmt.removeResource(ctResource);

}

@Nested
class AddCatcherAt {
@Test
void addsCatcherAtTheSpecifiedPosition() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: This test really tests three separate cases in one go:

  1. Inserting into an empty list.
  2. Inserting at the beginning of a non-empty list.
  3. Inserting at a position that is neither first nor last.

Given that this functionality is neither complicated nor critical to Spoon, that's probably fine. But it always bears thinking about how much is tested in any single test. The fact that we never insert at the end of a non-empty list also isn't a big deal considering what the implementation looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it always bears thinking about how much is tested in any single test.

Right. Ideally, we should test only one case per unit test. I will keep this in mind. We can skip refactoring for this feature because, like you said, it is not that critical and is rather a very simple feature.

The fact that we never insert at the end of a non-empty list also isn't a big deal considering what the implementation looks like.

Tests of addCatcher would already test that so we do not need to think about them I guess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests of addCatcher would already test that so we do not need to think about them I guess.

Precisely, "considering what the implementation looks like" :). White box testing has merits in that we can omit redundant testing, but it has drawbacks to match (e.g. creating tests based on the implementation rather than the specification).

// contract: the catcher should be added at the specified position
// arrange
Factory factory = createFactory();

CtTry tryStatement = factory.createTry();

CtCatch first = factory.createCatch();
first.setParameter(factory
.createCatchVariable()
.setType(factory.Type().createReference(IOException.class)));

CtCatch second = factory.createCatch();
second.setParameter(factory
.createCatchVariable()
.setMultiTypes(List.of(
factory.Type().createReference(ExceptionInInitializerError.class),
factory.Type().createReference(NoSuchFieldException.class))));

CtCatch third = factory.createCatch();
third.setParameter(factory
.createCatchVariable()
.setType(factory.Type().createReference(NoSuchMethodException.class)));
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 kind of a mouthful to read, and there is as far as I can tell unnecessary complexity in having a catcher that catches multiple types (I can't see how this influences insertion?).

I'd suggest something like this:

Suggested change
CtCatch first = factory.createCatch();
first.setParameter(factory
.createCatchVariable()
.setType(factory.Type().createReference(IOException.class)));
CtCatch second = factory.createCatch();
second.setParameter(factory
.createCatchVariable()
.setMultiTypes(List.of(
factory.Type().createReference(ExceptionInInitializerError.class),
factory.Type().createReference(NoSuchFieldException.class))));
CtCatch third = factory.createCatch();
third.setParameter(factory
.createCatchVariable()
.setType(factory.Type().createReference(NoSuchMethodException.class)));
CtCatch first = createCatch(factory, IOException.class);
CtCatch second = createCatch(factory, ExceptionInInitializerError.class);
CtCatch third = createCatch(factory, NoSuchMethodException.class);

With a helper like so:

		private <T> CtCatch createCatch(Factory factory, Class<T> typeToCatch) {
			CtTypeReference<T> typeReference = factory.Type().createReference(typeToCatch);
			return factory.createCatch().setParameter(
				factory.createCatchVariable().setType(typeReference)
			);
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

createCatch(Factory, Class )

Any reason why we pass the same instance of Factory and not create a new one whenever we want to call createCatch?


// act
tryStatement.addCatcherAt(0, third);
tryStatement.addCatcherAt(0, first);
tryStatement.addCatcherAt(1, second);
algomaster99 marked this conversation as resolved.
Show resolved Hide resolved

// assert
assertThat(tryStatement.getCatchers(), contains(first, second, third));
}

@Test
void throwsOutOfBoundsException_whenPositionIsOutOfBounds() {
// contract: `addCatcherAt` should throw an out-of-bounds exception when the specified position is out of
// bounds of the catcher collection
SirYwell marked this conversation as resolved.
Show resolved Hide resolved

// arrange
Factory factory = createFactory();

CtTry tryStatement = factory.createTry();
CtCatch catcherAtWrongPosition = factory.createCatch();
catcherAtWrongPosition.setParameter(factory
.createCatchVariable()
.setType(factory.Type().createReference(Exception.class)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also use the proposed helper here :)


// act & assert
assertThrows(IndexOutOfBoundsException.class,
() -> tryStatement.addCatcherAt(2, catcherAtWrongPosition));
algomaster99 marked this conversation as resolved.
Show resolved Hide resolved
}
}
}