openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.15k stars 322 forks source link

RemoveUnusedPrivateFields produce random field deletions in a multimodule project. #3219

Open rpau opened 1 year ago

rpau commented 1 year ago

Background https://rewriteoss.slack.com/archives/C01A843MWG5/p1683439528216859

We need to reproduce the issue we need to create a multimodule repository and apply RemoveUnusedPrivateFields multiple times.

Relates to #1753

timtebeek commented 1 year ago

Hi @jbessels ; we just pushed a new snapshot version of the Maven plugin with a potential fix. Would be so kind as to give it another try with version 4.46.0-SNAPSHOT?

<build>
    <plugins>
        <plugin>
            <groupId>org.openrewrite.maven</groupId>
            <artifactId>rewrite-maven-plugin</artifactId>
            <version>4.46.0-SNAPSHOT</version>
        </plugin>
    </plugins>
</build>

Given that it's been non deterministic, it's been rather hard to pin down. We did see this being more prevalent with a larger number of submodules, so we're hoping it was the linked ordering issue. Would you mind running a few times even if it works the first time?

jbessels commented 1 year ago

@timtebeek Copy/paste from Slack so the info does not get lost.

TL;DR behavior not changed. Did some testing. Ran it two times with all 96 recipes. Run 1 no change -> good. 2nd run it hang. Even after 30 minutes it still hung. I've noticed and reported this before. With 4.45 and say 25+ runs with all 96 recipes it blocked 2 times iirc. That was when I re tested it all and outcommented certain files. Only in the beginning it blocked. At the end it did not block anymore. That is till I outcommented the excludes for RemoveUnusedPrivateFields and it hang on run 2. But in the previous 10+ tests with RemoveUnusedPrivateFields enabled and outcommented files it just worked. So it rarely happens, BUT it happens. Decided to change tactics. Now only one recipe active: RemoveUnusedPrivateFields. At run 12 two out of 3 files were changed: SortableCollectionDataProvider.java and CRUDFormPage.java. The usage of LinkHashMap instead of HashMap has not changed the behavior. Perhaps a better approach is to look at the very specific situation where it fails. As said in the OP it ONLY goes wrong when a general <T> or <S> generic is used in the variable. In the Slack OP the class definition and the private fields are mentioned.

These are:

public abstract class CRUDFormPage<T extends CRUDObject<T>> extends ApplicationPage {
private final GenericTable<T, String> itemView; <-- explicitly set but removed sometimes

public class SortableCollectionDataProvider<T extends Serializable> extends SortableDataProvider<T, String> {
private final IModel<? extends Collection<T>> collection; <-- explicitly set but removed sometimes

public abstract class TimetableTrip<S extends TimetableStop> implements Comparable<TimetableTrip<S>> {
private final List<S> stops; <-- explicitly set but removed sometimes