Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RemoveMappersFix by using SafeDeleteFix #179

Merged
merged 1 commit into from
Mar 16, 2024

Conversation

hduelme
Copy link
Contributor

@hduelme hduelme commented Mar 11, 2024

@filiphr I found a working solution for #155
I used the SafeDeleteFix to delete the instance unlike my solution in #176 it checks for usage.

Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing @hduelme. Such a simple fix 😄. Thanks a lot for figuring this out.

One small thing, I think that the changes with the lists are not needed anymore. I don't think that createRemoveMappersFix will ever really return null. The failure that was visible in #162 is due to some other mistake with the reflection that I had done. I believe that the only change should be to extend from SafeDeleteFix and that's it. What do you think?

@hduelme
Copy link
Contributor Author

hduelme commented Mar 11, 2024

@filiphr the null return of createRemoveMappersFix could still happen.
For example:

@Mapper(componentModel = "spring")
abstract class WrongComponentModelMapper {

    static {
        Mappers.getMapper(WrongComponentModelMapper.class);
    }
    
    Target map(Target source);
}

or

@Mapper(config = MyConfig.class, componentModel = "spring")
interface WrongComponentModelMapper {


    WrongComponentModelMapper INSTANCE = () -> {
        return Mappers.getMapper(WrongComponentModelMapper.class);
    };

    Target map(Target source);
}

No variable to delete but the inspection still shows up.
Or while typing. This would cause a plugin crash if we don't check.

Alternatively we could change the behavior to always return the delete Option.

@filiphr
Copy link
Member

filiphr commented Mar 14, 2024

OK gotcha, I believe that this would have been a problem with the current version already. I don't think that we took such use cases into consideration.

I think that we can do this step by step.

  1. Make sure that the inspection and quick fix work on 2023.3 and later
  2. Add support for the 2 additional examples that you provided

What do you think @hduelme?

@hduelme
Copy link
Contributor Author

hduelme commented Mar 14, 2024

@filiphr Yes, we should solve it step by step. There should be no possible crashes in this version. So I would leaf it as it is for now. Would you open a issue for step 2?

@filiphr
Copy link
Member

filiphr commented Mar 16, 2024

Would you open a issue for step 2?

Yup, I've opened #183 for this.

I'll proceed with merging this one

@filiphr filiphr merged commit 8e25048 into mapstruct:main Mar 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants