Generated preconditions not correctly formatted

[Bananeweizen]

What is the smallest, simplest way to reproduce the problem?

Look at the locally generated RedundantCallRecipe.java for https://github.com/openrewrite/rewrite-migrate-java/blob/c223f325b13c1deb96eb96d6a6c9ff7570e5563c/src/main/java/org/openrewrite/java/migrate/lang/StringRules.java#L29-L33.

What did you see instead?

It contains this precondition:

            return Preconditions.check(
                    Preconditions.or(
                        Preconditions.and(
                        new UsesMethod<>("java.lang.String substring(..)"),
                        new UsesMethod<>("java.lang.String length(..)")
                    ),
                        new UsesMethod<>("java.lang.String substring(..)"),
                        new UsesMethod<>("java.lang.String toString(..)")
                    ),
                    javaVisitor
            );

The indentation inside the nested "AND" is wrong. This doesn't break the rewrite functionality, but it made me look for an error in the precondition generation which is not even there (I read this as two times the same top level condition and looked for why the duplicate is not recognized). :/

Are you interested in contributing a fix to OpenRewrite?

Maybe.

[timtebeek]

Thanks for the report! Can see how this is frustrating when debugging the generated recipe. We seem to already pass in an indentation argument when we join preconditions, but it'll need a closer look to know exactly where we're missing a level of indentation.

rewrite-templating/src/main/java/org/openrewrite/java/template/processor/RefasterTemplateProcessor.java

Lines 545 to 609 in 6fef187

	/* Generate the minimal precondition that would allow to match each before template individually. */
	@SuppressWarnings("SameParameterValue")
	@Nullable
	private String generatePreconditions(List<TemplateDescriptor> beforeTemplates, int indent) {
	Map<String, Set<String>> preconditions = new LinkedHashMap<>();
	for (TemplateDescriptor beforeTemplate : beforeTemplates) {
	int arity = beforeTemplate.getArity();
	for (int i = 0; i < arity; i++) {
	Set<String> usesVisitors = new LinkedHashSet<>();
	for (Symbol.ClassSymbol usedType : beforeTemplate.usedTypes(i)) {
	String name = usedType.getQualifiedName().toString().replace('$', '.');
	if (!name.startsWith("java.lang.") && !name.startsWith("com.google.errorprone.refaster.")) {
	usesVisitors.add("new UsesType<>(\"" + name + "\", true)");
	}
	}
	for (Symbol.MethodSymbol method : beforeTemplate.usedMethods(i)) {
	if (method.owner.getQualifiedName().toString().startsWith("com.google.errorprone.refaster.")) {
	continue;
	}
	String methodName = method.name.toString();
	methodName = methodName.equals("<init>") ? "<constructor>" : methodName;
	usesVisitors.add("new UsesMethod<>(\"" + method.owner.getQualifiedName().toString() + ' ' + methodName + "(..)\")");
	}
	preconditions.put(beforeTemplate.method.name.toString() + (arity == 1 ? "" : "$" + i), usesVisitors);
	}
	}
	if (preconditions.size() == 1) {
	return joinPreconditions(preconditions.values().iterator().next(), "and", indent + 4);
	} else if (preconditions.size() > 1) {
	Set<String> common = new LinkedHashSet<>();
	for (String dep : preconditions.values().iterator().next()) {
	if (preconditions.values().stream().allMatch(v -> v.contains(dep))) {
	common.add(dep);
	}
	}
	common.forEach(dep -> preconditions.values().forEach(v -> v.remove(dep)));
	preconditions.values().removeIf(Collection::isEmpty);
	if (common.isEmpty()) {
	return joinPreconditions(preconditions.values().stream().map(v -> joinPreconditions(v, "and", indent + 4)).collect(toList()), "or", indent + 4);
	} else {
	if (!preconditions.isEmpty()) {
	String uniqueConditions = joinPreconditions(preconditions.values().stream().map(v -> joinPreconditions(v, "and", indent + 12)).collect(toList()), "or", indent + 8);
	common.add(uniqueConditions);
	}
	return joinPreconditions(common, "and", indent + 4);
	}
	}
	return null;
	}
	private String joinPreconditions(Collection<String> preconditions, String op, int indent) {
	if (preconditions.isEmpty()) {
	return null;
	} else if (preconditions.size() == 1) {
	return preconditions.iterator().next();
	}
	char[] indentChars = new char[indent];
	Arrays.fill(indentChars, ' ');
	String indentStr = new String(indentChars);
	return "Preconditions." + op + "(\n" + indentStr + String.join(",\n" + indentStr, preconditions) + "\n" + indentStr.substring(0, indent - 4) + ')';
	}

[Bananeweizen]

Just from a short look I would think that line 608 is wrong (specifically the String.join(...) part). For nested calls the elements of the preconditions collection can be multiline strings. Therefore each of those elements would have to be split to lines, indented and joined again, not just the collection as a whole. Right now only the first line of a multiline string would be indented.

[timtebeek]

Thanks yes looks like it; had a brief look at changing the indentation, but no immediate fix from a quick attempt yet.

[timtebeek]

This has been revised in #120