#3861 Do not warn about unused Map source parameter used in an expression - #4102
#3861 Do not warn about unused Map source parameter used in an expression#4102renechoi wants to merge 2 commits into
Conversation
kdelay
left a comment
There was a problem hiding this comment.
The unprocessedSourceParameters analysis matches what I see in reportErrorForUnusedSourceParameters, and skipping the warning rather than removing the parameter from that set looks like the right minimal move.
One gap while checking the change: of the three expression kinds isReferencedFromJavaExpression guards, only getJavaExpression() is covered by a test. I removed the getDefaultJavaExpression() and getConditionJavaExpression() clauses (keeping the first) and ran the whole module:
./mvnw -pl processor -am test
Tests run: 3645, Failures: 0, Errors: 0, Skipped: 0
So neither clause currently has any regression protection. They are not dead code though. With those two clauses removed, both of these mappers emit MAPTOBEANMAPPING_WRONG_KEY_TYPE for values on the JDK and the Eclipse compiler, and with your branch as-is they compile clean:
@Mapper
public interface MapToBeanNonStringMapUsedInDefaultExpressionMapper {
MapToBeanNonStringMapUsedInDefaultExpressionMapper INSTANCE =
Mappers.getMapper( MapToBeanNonStringMapUsedInDefaultExpressionMapper.class );
@Mapping(target = "sum", source = "source.sum", defaultExpression = "java(calculateSum( values ))")
Target toTarget(Source source, Map<Integer, Integer> values);
default int calculateSum(Map<Integer, Integer> values) {
int sum = 0;
for ( Integer value : values.values() ) {
sum += value;
}
return sum;
}
// Source and Target are plain beans with a nullable Integer sum
}@Mapper
public interface MapToBeanNonStringMapUsedInConditionExpressionMapper {
MapToBeanNonStringMapUsedInConditionExpressionMapper INSTANCE =
Mappers.getMapper( MapToBeanNonStringMapUsedInConditionExpressionMapper.class );
@Mapping(target = "sum", source = "source.sum", conditionExpression = "java(!values.isEmpty())")
Target toTarget(Source source, Map<Integer, Integer> values);
// Source and Target are plain beans with a nullable Integer sum
}And the two test methods, dropped in next to shouldNotWarnIfMapParameterIsUsedInExpression:
@ProcessorTest
@IssueKey("3861")
@WithClasses({
MapToBeanNonStringMapUsedInDefaultExpressionMapper.class
})
void shouldNotWarnIfMapParameterIsUsedInDefaultExpression() {
Map<Integer, Integer> values = new HashMap<>();
values.put( 1, 10 );
values.put( 2, 20 );
MapToBeanNonStringMapUsedInDefaultExpressionMapper.Target target =
MapToBeanNonStringMapUsedInDefaultExpressionMapper.INSTANCE
.toTarget( new MapToBeanNonStringMapUsedInDefaultExpressionMapper.Source(), values );
assertThat( target.getSum() ).isEqualTo( 30 );
}
@ProcessorTest
@IssueKey("3861")
@WithClasses({
MapToBeanNonStringMapUsedInConditionExpressionMapper.class
})
void shouldNotWarnIfMapParameterIsUsedInConditionExpression() {
MapToBeanNonStringMapUsedInConditionExpressionMapper.Source source =
new MapToBeanNonStringMapUsedInConditionExpressionMapper.Source();
source.setSum( 5 );
MapToBeanNonStringMapUsedInConditionExpressionMapper.Target target =
MapToBeanNonStringMapUsedInConditionExpressionMapper.INSTANCE
.toTarget( source, new HashMap<>() );
assertThat( target.getSum() ).isNull();
}Measured on FromMapMappingTest: 52 run / 0 failures on your branch, 52 run / 4 failures with the two clauses removed (both compilers, both tests). JDK 26 locally. Take them as-is if useful.
One thing I am unsure about rather than objecting to: the match is on raw expression text, so a parameter named values also counts as referenced in java(load("values")) or java(other.values). You call the trade-off out in the description, and the direction is safe, but would it be worth a short code comment at isReferencedBy so it does not read as an oversight later?
|
Thanks, the gap is real and I reproduced it before touching anything: with both extra clauses dropped and no new tests, Added both cases in 8e1d51b. I wrote my own fixtures rather than taking yours as-is, so that each test also pins what the expression does instead of only that it compiles:
Negative control per clause,
Each failure is the warning from the issue: Full module on the new head is 3649 run / 0 failures, and On the raw text match: you are right that |
A
Mapsource parameter that is only referenced from a Java expression is currently reported as unused.BeanMappingMethod#reportErrorForUnusedSourceParameterswarns for every unprocessedMapsource parameter whose key type is notString, on the assumption that the user intended a bean-from-map mapping and got the type wrong. A parameter that is passed to a method from@Mapping(expression = "java(...)")never becomes a processed source parameter, so it hits that branch even though it is used.For the mapper from the issue:
MapStruct emits:
The generated code is correct; only the warning is wrong.
Change
Before emitting the warning, check whether the parameter name appears as an identifier in any of the method's Java expressions (
expression,defaultExpression,conditionExpression). If it does, the parameter is in use and the warning is skipped. Everything else is untouched: the parameter stays inunprocessedSourceParameters, so property mapping, unmapped-source reporting and parameter-name based mapping behave exactly as before.The check is intentionally a word-boundary match on the raw expression text rather than a parse. It can only ever suppress a warning, never introduce one.
Verification
Run from the repository root with JDK 21.
MapToBeanNonStringMapUsedInExpressionMapperplusFromMapMappingTest#shouldNotWarnIfMapParameterIsUsedInExpression.@ProcessorTestfails on any unexpected diagnostic, so the new test reproduces the issue.main(with only the test added) that test fails for both the javac and the eclipse compiler with exactly the warning quoted above:Tests run: 48, Failures: 2../mvnw -pl processor test -Dtest=FromMapMappingTest->Tests run: 48, Failures: 0, Errors: 0.processormodule suite:./mvnw -pl processor test->Tests run: 3645, Failures: 0, Errors: 0, Skipped: 0,BUILD SUCCESS.Fixes #3861