feat: Add `CtTry.addCatcherAt` #4954

algomaster99 · 2022-10-12T09:10:17Z

Reference: #3884

The order of catchers is important because the thrown exception is matched with the parameters of the catchers in the order they appear, hence it is semantically important.

algomaster99 · 2022-10-12T10:09:59Z

@slarse please have a look at this when you get time.

src/test/java/spoon/test/trycatch/TryCatchTest.java

algomaster99 · 2022-10-12T12:24:07Z

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

+	 * @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);


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.

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

slarse

Some notes and some stuff I think we should change :)

slarse · 2022-10-12T18:39:05Z

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

+	 * @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);


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.

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.

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 :)

slarse · 2022-10-12T18:40:35Z

src/main/java/spoon/support/reflect/code/CtTryImpl.java

+	}
+
+	@Override
+	public <T extends CtTry> T addCatcherAt(int position, CtCatch catcher) {


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.

src/test/java/spoon/test/trycatch/TryCatchTest.java

slarse · 2022-10-12T19:01:34Z

src/test/java/spoon/test/trycatch/TryCatchTest.java

+			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)));


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) ); }

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?

slarse · 2022-10-12T19:05:49Z

src/test/java/spoon/test/trycatch/TryCatchTest.java

+	@Nested
+	class AddCatcherAt {
+		@Test
+		void addsCatcherAtTheSpecifiedPosition() {


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

Inserting into an empty list.

Inserting at the beginning of a non-empty list.

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.

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.

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).

slarse · 2022-10-12T19:06:27Z

src/test/java/spoon/test/trycatch/TryCatchTest.java

+			CtCatch catcherAtWrongPosition = factory.createCatch();
+			catcherAtWrongPosition.setParameter(factory
+					.createCatchVariable()
+					.setType(factory.Type().createReference(Exception.class)));


Also use the proposed helper here :)

src/test/java/spoon/test/trycatch/TryCatchTest.java

Co-authored-by: Simon Larsén <slarse@slar.se>

sonatype-lift · 2022-10-13T11:15:06Z

src/main/java/spoon/support/reflect/code/CtTryImpl.java

+	}
+
+	@Override
+	public <T extends CtTry> T addCatcherAt(int position, CtCatch catcher) {
 		if (catcher == null) {
 			return (T) this;


💬 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 ]

Co-authored-by: Simon Larsén <slarse@slar.se>

algomaster99 · 2022-10-13T11:43:34Z

I pushed an empty commit to give @slarse co-authorship, but now I realise that I have accepted some of your suggestions so you will be a co-author anyway if this PR gets squashed and merged 😅

algomaster99 · 2022-10-13T11:56:47Z

src/test/java/spoon/test/trycatch/TryCatchTest.java

+					() -> tryStatement.addCatcherAt(1, catcherAtWrongPosition));
+		}
+
+		private <T extends Throwable> CtCatch createCatch(Factory factory, Class<T> typeToCatch) {


I made it type stricter by replacing T with <T extends Throwable>

slarse · 2022-10-16T20:45:34Z

I think it will be a couple of days before I can return to this, I was away during the weekend which ate up most of what would normally be my OSS time :)

algomaster99 · 2022-10-17T09:52:51Z

Understandable. Take your time. Not very urgent. :)

slarse

Thanks @algomaster99 , sorry about the delay! Turned out I was away this weekend as well :s

slarse · 2022-10-23T18:55:09Z

src/test/java/spoon/test/trycatch/TryCatchTest.java

+	@Nested
+	class AddCatcherAt {
+		@Test
+		void addsCatcherAtTheSpecifiedPosition() {


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).

slarse · 2022-10-23T18:55:41Z

src/test/java/spoon/test/trycatch/TryCatchTest.java

+					() -> tryStatement.addCatcherAt(1, catcherAtWrongPosition));
+		}
+
+		private <T extends Throwable> CtCatch createCatch(Factory factory, Class<T> typeToCatch) {


algomaster99 added 2 commits October 12, 2022 10:59

Add method to add catcher at a specified position in the try statement

b7492a6

Add tests

3109fa1

algomaster99 changed the title ~~feat: Add CtTry.addCatchAt~~ feat: Add CtTry.addCatcherAt Oct 12, 2022

Add Javadoc

8bc0965

algomaster99 force-pushed the add-catcher-at branch from 2cb5e87 to 8bc0965 Compare October 12, 2022 09:29

algomaster99 mentioned this pull request Oct 12, 2022

chore(deps): update SonarJava dependency ASSERT-KTH/sorald#900

Merged

2 tasks

SirYwell reviewed Oct 12, 2022

View reviewed changes

src/test/java/spoon/test/trycatch/TryCatchTest.java Show resolved Hide resolved

Improve Javadoc

44f4d89

algomaster99 commented Oct 12, 2022

View reviewed changes

algomaster99 requested a review from SirYwell October 12, 2022 12:24

slarse suggested changes Oct 12, 2022

View reviewed changes

Update src/test/java/spoon/test/trycatch/TryCatchTest.java

0510d3e

Co-authored-by: Simon Larsén <slarse@slar.se>

sonatype-lift bot reviewed Oct 13, 2022

View reviewed changes

algomaster99 and others added 3 commits October 13, 2022 13:21

Update src/test/java/spoon/test/trycatch/TryCatchTest.java

3b7bd25

Co-authored-by: Simon Larsén <slarse@slar.se>

Incorporate changes

1401c80

Give co-authorship to @slarse

0a4c803

Co-authored-by: Simon Larsén <slarse@slar.se>

algomaster99 commented Oct 13, 2022

View reviewed changes

algomaster99 requested review from slarse and removed request for SirYwell October 15, 2022 16:13

slarse approved these changes Oct 23, 2022

View reviewed changes

slarse merged commit ca87730 into INRIA:master Oct 23, 2022

algomaster99 deleted the add-catcher-at branch October 30, 2022 07:35

monperrus mentioned this pull request Mar 9, 2023

release Spoon 10.3.0 #5135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `CtTry.addCatcherAt` #4954

feat: Add `CtTry.addCatcherAt` #4954

algomaster99 commented Oct 12, 2022

algomaster99 commented Oct 12, 2022

algomaster99 Oct 12, 2022

slarse Oct 12, 2022

slarse left a comment

slarse Oct 12, 2022

algomaster99 Oct 13, 2022

slarse Oct 13, 2022

slarse Oct 12, 2022

slarse Oct 12, 2022

algomaster99 Oct 13, 2022

slarse Oct 12, 2022

algomaster99 Oct 13, 2022

slarse Oct 23, 2022

slarse Oct 12, 2022

sonatype-lift bot Oct 13, 2022

algomaster99 commented Oct 13, 2022 •

edited

Loading

algomaster99 Oct 13, 2022

slarse Oct 23, 2022

slarse commented Oct 16, 2022

algomaster99 commented Oct 17, 2022

slarse left a comment

slarse Oct 23, 2022

slarse Oct 23, 2022

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

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

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

feat: Add CtTry.addCatcherAt #4954

feat: Add CtTry.addCatcherAt #4954

Conversation

algomaster99 commented Oct 12, 2022

algomaster99 commented Oct 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Oct 13, 2022

Choose a reason for hiding this comment

algomaster99 commented Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse commented Oct 16, 2022

algomaster99 commented Oct 17, 2022

slarse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Add `CtTry.addCatcherAt` #4954

feat: Add `CtTry.addCatcherAt` #4954

algomaster99 commented Oct 13, 2022 •

edited

Loading