#163 Reference support for @InheritConfiguration and @InheritInverseConfiguration

[thunderhook]

Resolves #163

Provided two test cases with multiple completion candidates.

[thunderhook]

I tried to provide this kind of base reference on the level of MapstructBaseReference but it would have led to more duplication. Therefore I decided to provide a base reference on this level.

[filiphr]

I'm not sure that I understand what MapStructNonNestedBaseReference means.

[thunderhook]

It is a reference that does not support nesting, see:

• mapstruct-idea/src/main/java/org/mapstruct/intellij/codeinsight/references/MapstructBaseReference.java

  Lines 169 to 170 in 88ebd38

  	static <T extends MapstructBaseReference> PsiReference[] create(PsiElement psiElement,
  	ReferenceCreator<T> creator, boolean supportsNested) {

This is called with true from source or target references:

• MapstructSourceReference
• MapstructTargetReference

Then it is a reference that supports the nested, dotted notation like property.nested.field. So this is possible if the reference is about a type, I guess?

It is false, when nesting is not supported, so in MapstructMappingInheritConfigurationReference, MapstructMappingInheritInverseConfigurationReference, MapstructMappingQualifiedByNameReference where the references are not nested, like method names. And this base class is for those candidates.

Got a better naming idea? I also called it MapstructUntypedBaseReference, but that was even worse.

[filiphr]

It feels to me like we need a type like MapStructPropertyReference which would be for the source and target references (which are the only ones where we have nested properties. I wouldn't mix the nested word though in the types, it's a bit strange to me

[filiphr]

Nice work @thunderhook.

I had a look at it, and I'm not sure that I like the new things in mergeInheritedOptions. It feels like we are doing multiple things in the same time, when we actually only need one thing. Perhaps we need to refactor that to only get what we actually need instead of trying to fit everything into the InheritConfigurationContext. If feels more like we need methods that would return exactly what we need instead of having mappedTargets, forwardTemplateCandidates and inverseTemplateCandidate in one place when most of them aren't needed anyways.

[filiphr]

The referenced issues is from MapStruct, right? It is mapstruct/mapstruct#1013? Perhaps we need to put that link directly here.

[thunderhook]

Yes, good catch, used the example from the mapstruct repository and that was copy/pasted.

[filiphr]

I'm not sure that I understand what MapStructNonNestedBaseReference means.

[thunderhook]

I had a look at it, and I'm not sure that I like the new things in mergeInheritedOptions. It feels like we are doing multiple things in the same time, when we actually only need one thing. Perhaps we need to refactor that to only get what we actually need instead of trying to fit everything into the InheritConfigurationContext. If feels more like we need methods that would return exactly what we need instead of having mappedTargets, forwardTemplateCandidates and inverseTemplateCandidate in one place when most of them aren't needed anyways.

I feel you. I would have loved to implement it this way. However, the current logic has been extracted (and adapted) from the original MapStruct source (MapperCreationProcessor). This is how the inheritance is evaluated there. Writing a new implementation could lead to false positives.

For me, at least, I think it is better not to have a completion than to have a possible wrong one that leads to frustration.

I think if the inheritance logic is to be refactored, it should be done in the MapStruct source first, as there is a lot more code coverage there. But even then, I personally would be too scared to touch it. Maybe it is because of a performance optimisation by not calculating things multiple times.

WDYT? Should I try to rewrite it? Maybe I can find some time in July/August.

---

Not sure why my comments are not listed directly under your comments, sorry for the confusion.

[filiphr]

I feel you. I would have loved to implement it this way. However, the current logic has been extracted (and adapted) from the original MapStruct source (MapperCreationProcessor). This is how the inheritance is evaluated there. Writing a new implementation could lead to false positives.

I understand what you are saying. However, I think that there is a big difference in the possible optimizations for the annotation processor vs the IntelliJ plugin. The plugin runs in the scope of the user view and at the time the file is displayed to the user. We should try to be as optimal as possible there, we do not want to make IntelliJ sluggish.

If you want I can look into refactoring those bits?

[thunderhook]

If you want I can look into refactoring those bits?

Yes, you're welcome to do that. I'm curious to see what it looks like after the refactoring. 👍