fix: print each resource in `CtTryWithResource` exactly once and retain separator #4309

algomaster99 · 2021-11-25T15:02:36Z

The second resource in the catch block, BufferedWriter writer = newBufferedWriter(outputFilePath, charset), is printed twice. For example,

        try (ZipFile zf = new ZipFile(zipFileName);
             BufferedWriter writer = newBufferedWriter(outputFilePath, charset)) { }

is changed to

        try (ZipFile zf = new ZipFile(zipFileName);
             BufferedWriter writer = newBufferedWriter(outputFilePath, charset);BufferedWriter writer = newBufferedWriter(outputFilePath, charset)) { }

when printed using the sniper printer.

I think it is happening because we add a separator, ;, after first BufferedWriter writer = newBufferedWriter(outputFilePath, charset) which changes muted from true to false. Although the separator is not needed here, it is still printed. This may be similar to #4306 .

Please note that muted should remain true till we print the second resource because all resources are printed at once because of this line here. Basically, if a collection fragment is not modified, we print it at once and set muted to true for upcoming elements in the collection.

algomaster99 · 2021-11-25T15:59:08Z

The context is pushed before printing the resources, however that same context is not used when we are printing the separator. Apparently, sfc.knowsHowToPrint returns false when we are printing ;. Probably, the problem lies there.

algomaster99 · 2021-11-26T14:40:43Z

The context is pushed before printing the resources, however that same context is not used when we are printing the separator. Apparently, sfc.knowsHowToPrint returns false when we are printing ;. Probably, the problem lies there.

I went along these lines and pushed a fix for it.

I debugged where the context was lost and it was in knowsHowToPrint method. With the changes, I am ensuring that context doesn't get lost if the sniper printer is printing a ; while it is printing the resources. I have a feeling that such a fix will also help patch #4306 .

One major problem I see in this patch is that I am hard-coding ; in an abstract code that doesn't sound logically coherent. There is a probably a better way to do it and I hope to discover that through reviews.

@slarse @MartinWitt @monperrus , could you please review it?

MartinWitt · 2021-11-27T02:25:17Z

src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentContextCollection.java

+		List<SourceFragment> elementSourceFragments = childFragments.stream()
+				.filter(fragment -> fragment instanceof ElementSourceFragment)
+				.collect(Collectors.toList());
+		for (SourceFragment sourceFragment: elementSourceFragments) {
+			ElementSourceFragment elementSourceFragment = ((ElementSourceFragment) sourceFragment);
+			return elementSourceFragment.getRoleInParent() == role;
+		}
+		return false;
+	}


This is really hard to read and should be refactored in a single loop. A loop that always returns and doesn't loop is kinda unexpected.

slarse · 2021-11-27T09:23:08Z

src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentContextCollection.java

+			} else if (tpe.getToken().equals(";")) {
+				if (checkIfPartOfRole(CtRole.TRY_RESOURCE)) {


Collapse

Suggested change

} else if (tpe.getToken().equals(";")) {

if (checkIfPartOfRole(CtRole.TRY_RESOURCE)) {

} else if (tpe.getToken().equals(";") && checkIfPartOfRole(CtRole.TRY_RESOURCE)) {

slarse · 2021-11-27T09:26:34Z

src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentContextCollection.java

@@ -68,6 +73,17 @@ public boolean knowsHowToPrint(PrinterEvent event) {
 		throw new SpoonException("Unexpected PrintEvent: " + event.getClass());
 	}

+	private boolean checkIfPartOfRole(CtRole role) {


Just reading the invocation of this method, I have no idea of what its intention is. Check if what is part of a role? What does it mean to be part of a role?

check is also usually reserved for methods that throw. For methods that are meant to return a boolean, I'd recommend starting with is, has etc.

Looking at the method body, this method returns true if the first ElementSourceFragment of childFragments matches the provided role. Otherwise it returns false. Can you explain those semantics? I also agree with @MartinWitt that the method needs refactoring, but first I want to try to understand its intent.

algomaster99 · 2021-11-29T12:23:38Z

@MartinWitt @slarse The intent of the method is to check if the role of the current ElementSourceFragments of childFragments is the passed role (in our case - CtRole.TRY_RESOURCE).

Can you explain those semantics?

I assumed that if the first element has the role CtRole.TRY_RESOURCE, then the rest elements will also have the same role and I guess it's safe to do so because you can only used objects inside it which implement java.lang.AutoCloseable. Thus, making all of them TRY_RESOURCE.

I have explained why this code was required in the first place here.

algomaster99 · 2021-11-29T12:37:50Z

I have refactored the method according to what @slarse and @MartinWitt suggested. I named the method childFragmentHasSpecifiedRoleInParent because I think it describes the intent clearly although it doesn't start with has, does, or is.

slarse

Added a further suggestion for clarification now that I understand the intent.

slarse · 2021-11-29T21:49:06Z

src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentContextCollection.java

+	private boolean childFragmentHasSpecifiedRoleInParent(CtRole roleInParent) {
+		Optional<SourceFragment> optionSourceFragment = childFragments.stream()
+				.filter(fragment -> fragment instanceof ElementSourceFragment)
+				.findFirst();
+		if (optionSourceFragment.isPresent()) {
+			ElementSourceFragment elementSourceFragment = (ElementSourceFragment) optionSourceFragment.get();
+			return  elementSourceFragment.getRoleInParent() == roleInParent;
+		}
+		return false;
+	}


I assumed that if the first element has the role CtRole.TRY_RESOURCE, then the rest elements will also have the same role and I guess it's safe to do so because you can only used objects inside it which implement

That's probably safe, yes. The thingy that builds the CollectionSourceFragment (which is later fed into the collection context) groups the children by role. I don't have a great understanding of how often this check will run, but I don't think a solitary ; is printed very often. In the entire sniper printer unit test suite, it only happens 4 times, 2 of which are in your tests. So probably this is fine from a performance perspective, but it bears keeping in mind.

Reading the method title, I'm still thinking "which child fragment?". And the answer to that is of course any child fragment. I would therefore suggest a renaming like below, and also further idiomatic use of the stream.

Suggested change

private boolean childFragmentHasSpecifiedRoleInParent(CtRole roleInParent) {

Optional<SourceFragment> optionSourceFragment = childFragments.stream()

.filter(fragment -> fragment instanceof ElementSourceFragment)

.findFirst();

if (optionSourceFragment.isPresent()) {

ElementSourceFragment elementSourceFragment = (ElementSourceFragment) optionSourceFragment.get();

return elementSourceFragment.getRoleInParent() == roleInParent;

}

return false;

}

private boolean anyChildFragmentHasRole(CtRole role) {

return childFragments.stream()

.filter(ElementSourceFragment.class::isInstance)

.map(ElementSourceFragment.class::cast)

.map(ElementSourceFragment::getRoleInParent)

.map(role::equals)

.findAny()

.orElse(false);

}

I'm using findAny() here to emphasize that there's nothing special about the first element, we just assume that if any element has a given role, the others do too.

slarse · 2021-11-29T21:55:07Z

src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentContextCollection.java

@@ -52,6 +53,8 @@ public boolean knowsHowToPrint(PrinterEvent event) {
 				return false;
 			} else if (tpe.getType() == TokenType.IDENTIFIER) {
 				return findIndexOfNextChildTokenByType(TokenType.IDENTIFIER) >= 0;
+			} else if (tpe.getToken().equals(";") && childFragmentHasSpecifiedRoleInParent(CtRole.TRY_RESOURCE)) {


With the suggested renaming, this becomes somewhat easier to interpret, I think:

Suggested change

} else if (tpe.getToken().equals(";") && childFragmentHasSpecifiedRoleInParent(CtRole.TRY_RESOURCE)) {

} else if (tpe.getToken().equals(";") && anyChildFragmentHasRole(CtRole.TRY_RESOURCE)) {

algomaster99 · 2021-11-30T08:39:24Z

So probably this is fine from a performance perspective, but it bears keeping in mind.

We can add a comment for this because it might confound future developers to see such a thing. It's true for future-me at least.

And the answer to that is of course any child fragment.

Right. We could do this with anyMatch. Thank you for refactoring the stream. I often get confused about how to club the intermediate operations. I am working on it.

I will push the changes.

algomaster99 · 2021-11-30T08:50:21Z

@slarse @MartinWitt incorporated your changes. I have also added a short comment explaining the else-if block. Please review.

MartinWitt · 2021-11-30T12:56:33Z

LGTM, if @slarse is fine with it, we can merge.

algomaster99 · 2021-12-01T16:06:24Z

@slarse reminder to merge this PR, if you are okay with the changes. :)

Reproduce failing test case for ASSERT-KTH/sorald#600

b5786f6

algomaster99 added 4 commits November 26, 2021 14:58

Check if ; is part of try with resource statement

3716cd2

Add test case for checking if ; is retained

dce2e04

Remove redundant import

da26d0e

Refactor test cases into two files

d5b1964

algomaster99 marked this pull request as ready for review November 26, 2021 14:40

algomaster99 changed the title ~~fix: print each resource in CtTryWithResource exactly once~~ fix: print each resource in CtTryWithResource exactly once and retain separator Nov 26, 2021

MartinWitt reviewed Nov 27, 2021

View reviewed changes

slarse suggested changes Nov 27, 2021

View reviewed changes

algomaster99 added 2 commits November 29, 2021 13:32

Refactor method to clarify intent

224ea1a

Refactor method name to describe what it does

bf3d386

slarse suggested changes Nov 29, 2021

View reviewed changes

slarse reviewed Nov 29, 2021

View reviewed changes

algomaster99 added 2 commits November 30, 2021 09:48

Refactor stream and add a comment to explain the else-if condition

e8d7726

Add link to PR

916249c

Please checkstyle

8e3fb9c

monperrus mentioned this pull request Nov 30, 2021

Duplicate variable definition ASSERT-KTH/sorald#600

Closed

slarse approved these changes Dec 1, 2021

View reviewed changes

slarse merged commit cc935df into INRIA:master Dec 1, 2021

algomaster99 mentioned this pull request Dec 9, 2021

chore: upgrade Spoon to 10.0.1-beta-2 ASSERT-KTH/sorald#676

Merged

algomaster99 deleted the try-with-resource branch December 21, 2021 14:16

monperrus mentioned this pull request Mar 14, 2022

WIP release: next release #4636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: print each resource in `CtTryWithResource` exactly once and retain separator #4309

fix: print each resource in `CtTryWithResource` exactly once and retain separator #4309

algomaster99 commented Nov 25, 2021 •

edited

Loading

algomaster99 commented Nov 25, 2021

algomaster99 commented Nov 26, 2021 •

edited

Loading

MartinWitt Nov 27, 2021

slarse Nov 27, 2021

slarse Nov 27, 2021

algomaster99 commented Nov 29, 2021

algomaster99 commented Nov 29, 2021 •

edited

Loading

slarse left a comment

slarse Nov 29, 2021

slarse Nov 29, 2021

algomaster99 commented Nov 30, 2021

algomaster99 commented Nov 30, 2021

MartinWitt commented Nov 30, 2021

algomaster99 commented Dec 1, 2021

		} else if (tpe.getToken().equals(";")) {
		if (checkIfPartOfRole(CtRole.TRY_RESOURCE)) {

	} else if (tpe.getToken().equals(";")) {
	if (checkIfPartOfRole(CtRole.TRY_RESOURCE)) {
	} else if (tpe.getToken().equals(";") && checkIfPartOfRole(CtRole.TRY_RESOURCE)) {

fix: print each resource in CtTryWithResource exactly once and retain separator #4309

fix: print each resource in CtTryWithResource exactly once and retain separator #4309

Conversation

algomaster99 commented Nov 25, 2021 • edited Loading

algomaster99 commented Nov 25, 2021

algomaster99 commented Nov 26, 2021 • edited Loading

MartinWitt Nov 27, 2021

Choose a reason for hiding this comment

slarse Nov 27, 2021

Choose a reason for hiding this comment

slarse Nov 27, 2021

Choose a reason for hiding this comment

algomaster99 commented Nov 29, 2021

algomaster99 commented Nov 29, 2021 • edited Loading

slarse left a comment

Choose a reason for hiding this comment

slarse Nov 29, 2021

Choose a reason for hiding this comment

slarse Nov 29, 2021

Choose a reason for hiding this comment

algomaster99 commented Nov 30, 2021

algomaster99 commented Nov 30, 2021

MartinWitt commented Nov 30, 2021

algomaster99 commented Dec 1, 2021

fix: print each resource in `CtTryWithResource` exactly once and retain separator #4309

fix: print each resource in `CtTryWithResource` exactly once and retain separator #4309

algomaster99 commented Nov 25, 2021 •

edited

Loading

algomaster99 commented Nov 26, 2021 •

edited

Loading

algomaster99 commented Nov 29, 2021 •

edited

Loading