[Bug] Erroenous AST for type in a Jigsaw module

[Auties00]

Describe the bug In short, calling CtElement<P extends CtElement>#getParent(P) using as a parameter CtModule.class always returns the unnamed module even when using types such as String that are definitely in a valid jigsaw module. You can find an example below and various snippets I tried.

To Reproduce

Input:

public class TestInput {
    private static final String ABC = String.valueOf("");
}

Processing with Spoon:

public class FieldTransformer extends AbstractProcessor<CtField<?>> {
    @Override
    public boolean isToBeProcessed(CtField<?> element) {
        return true;
    }

    @Override
    public void process(CtField<?> element) {
        if (element.getAssignment() == null) {
            return;
        }

        element.getParent(CtModule.class); // Unnamed module 
        element.getType().getParent(CtModule.class); // Unnamed module 
        element.getType().getPackage().getParent(CtModule.class); // Unnamed module 
    }
}

• OS: Windows 11
• JDK: openjdk version "16" 2021-03-16
• Spoon version: 10.2.0-beta-10

[SirYwell]

It would be good to know your Launcher configuration.

Also, note that the parent of a Ct*Reference is the context it appears in. In your example, the code should return the module the TestInput class belongs to for all cases.

[Auties00]

It would be good to know your Launcher configuration.

Also, note that the parent of a Ct*Reference is the context it appears in. In your example, the code should return the module the TestInput class belongs to for all cases.

Here it is:

        var launcher = new Launcher();
        launcher.addInputResource(".../TestInput.class");
        launcher.setSourceOutputDirectory(".../test-classes/");
        launcher.addProcessor(new FieldTransformer());
        launcher.run();

(The path is not complete, but I can guarantee that both the input file and output directory are recognized correctly). Also, even TestInput is in a module, here is the module-info:

module it.auties.example {
    requires spoon.core;
}

So I'm almost sure that this is a bug. Also is there any way to hypothetically get the module for the type like I was trying to do?

EDIT: Btw I've noticed that calling element.getFactory().Module().getAllModules() returns a list containing only the unnamed module even when other modules are clearly loaded

SECOND EDIT: Thinking about it, there's no way for the launcher to know about the module where TestInput is as it's not listed as a source so I guess that it's behaving correctly. I'd like to know how to add it correctly so that I can test if it works and if it's possible to find the module for, for example, the String type

THIRD EDIT: element.getType().getActualClass().getModule() works perfectly though getActualClass is deprecated and the replacement, getTypeDeclaration, seems to return the wrong module:

[SirYwell]

You need to set the compliance level to >=9, (e.g. launcher.getEnvironment().setComplianceLevel(9)). Otherwise, spoon should throw an exception as soon as it loads a module-info file. This file must be present in the input sources (I'd personally recommend to just pass the source folder as input resource).

However, there are two additional issues:

1. As soon as you print the module (calling toString() on it, even if it's just indirectly called by the debugger), you might encounter the issue that will be fixed by the PR linked above.
2. The java.lang.String class isn't available as input resource either most likely - spoon will create a shadow model for it, containing information available via reflection. Currently, modules aren't supported there, so it will always return the unnamed module for shadow classes. Using getActualClass().getModule(), as you mentioned, is a workaround for that for now I guess.

[Auties00]

You need to set the compliance level to >=9, (e.g. launcher.getEnvironment().setComplianceLevel(9)). Otherwise, spoon should throw an exception as soon as it loads a module-info file. This file must be present in the input sources (I'd personally recommend to just pass the source folder as input resource).

However, there are two additional issues:

1. As soon as you print the module (calling toString() on it, even if it's just indirectly called by the debugger), you might encounter the issue that will be fixed by the PR linked above.
2. The java.lang.String class isn't available as input resource either most likely - spoon will create a shadow model for it, containing information available via reflection. Currently, modules aren't supported there, so it will always return the unnamed module for shadow classes. Using getActualClass().getModule(), as you mentioned, is a workaround for that for now I guess.

Thanks very much. I could implement that feature and open a PR if you'd like btw

[SirYwell]

Sure! Feel free to give it a try, and don't hesitate to ask questions.

[Auties00]

Sure! Feel free to give it a try, and don't hesitate to ask questions.

Just one question, I've noticed there are a bunch of raw types. Is this intended? I've looked at the generic signatures of all the methods and there are some that make using the model quite hard in a non-generic method in my opinion. It's not even relevant to this PR btw

[I-Al-Istannen]

Just one question, I've noticed there are a bunch of raw types. Is this intended? I've looked at the generic signatures of all the methods and there are some that make using the model quite hard in a non-generic method in my opinion. It's not even relevant to this PR btw

This is a relict of the past and I am pretty sure we wouldn't do it this way today. The generics make using the model a bit more convenient if you statically know the types and they are on the classpath, which isn't how many people use spoon. The decision was made way before I had anything to do with this project, so maybe somebody else has a nicer story :)

I think you will just have to live with the existing ones for now though, unless you find a nice solution to remove them in a backwards-compatible way. Due to the interesting signatures of many methods, rawtypes are sometimes the only way to express what you want. In some places I suspect the authors of the code just did not bother, as the generic type isn't relevant for most things anyways.

[Auties00]

Just one question, I've noticed there are a bunch of raw types. Is this intended? I've looked at the generic signatures of all the methods and there are some that make using the model quite hard in a non-generic method in my opinion. It's not even relevant to this PR btw

This is a relict of the past and I am pretty sure we wouldn't do it this way today. The generics make using the model a bit more convenient if you statically know the types and they are on the classpath, which isn't how many people use spoon. The decision was made way before I had anything to do with this project, so maybe somebody else has a nicer story :)

I think you will just have to live with the existing ones for now though, unless you find a nice solution to remove them in a backwards-compatible way. Due to the interesting signatures of many methods, rawtypes are sometimes the only way to express what you want. In some places I suspect the authors of the code just did not bother, as the generic type isn't relevant for most things anyways.

Yeah, imo the original designers tried to use generics in a way that is not really intended. I understand what they were going for, but it's definitely bad design imo. I also doubt there's any way to fix this in a backwards compatible way. Btw I'm almost done implementing the feature. Another question though: is it mandatory for all public methods to remain there? I have kept them all, but deprecated a few and I'm not sure if that's what I'm supposed to do

[SirYwell]

is it mandatory for all public methods to remain there? I have kept them all, but deprecated a few and I'm not sure if that's what I'm supposed to do

Basically, see this section in the CONTRIBUTING.md. If the methods are internal, you can remove them, but for anything else deprecating is the preferred way. I think everything else depends on the actual method and what alternatives are feasible.

[slarse]

Yeah, imo the original designers tried to use generics in a way that is not really intended. I understand what they were going for, but it's definitely bad design imo.

The use of generics in the API is well before my time as well, but the I've heard the story of how it came to be. It more or less sums up to "it seemed like a good idea on paper". We've since realized that it wasn't, and have started down a road where new APIs don't receive the generic treatment. And that's without putting down the original contributors; hindsight is 20/20 after all.

There are also definitely places where generics have been outright misused, such as in return types where the caller gets to decide what the return type is (which makes absolutely zero sense).

I also doubt there's any way to fix this in a backwards compatible way.

I'm actually pretty certain there is a way. The thing with generics in Java is that they are a compile time feature, and the compiler can be persuaded to do a great many things using the dark arts (a.k.a unsafe casts). Something that's been on my TODO list for the past year or so is to start prototyping a V2 API with most generics removed, and derive the V1 API from that using (among other things) annotations, Spoon itself and liberal sprinkles of the dark arts. Essentially, the V2 API would be the "real deal", and the V1 API wrapper around the V2 API with retained generics. Some hiccups might exist, such as where there's nasty use of generics in return types, which may necessitate some minor breakage to clean up.

Finding the time to get around to this has proven to be a little bit on the tricky side of things, though. But I figure I'll get there eventually.

[Auties00]

The use of generics in the API is well before my time as well, but the I've heard the story of how it came to be. It more or less sums up to "it seemed like a good idea on paper". We've since realized that it wasn't, and have started down a road where new APIs don't receive the generic treatment. And that's without putting down the original contributors; hindsight is 20/20 after all.

I can understand why it seemed a good idea, I thought the same when I used the API for the first time. Though after some ClassCastExceptions and especially after looking at the source, it's clear that it was a bad idea. Still a nice try though

There are also definitely places where generics have been outright misused, such as in return types where the caller gets to decide what the return type is (which makes absolutely zero sense).

It actually makes sense and I think that this makes the design choice a nice try. I would guess that the original authors came from a weakly typed programming language(aka implicit type coercion with bounds) and wanted to reproduce the same concept in Java, which looks very nice on paper, but is actually terrible in practice as generics were not intended for this.

I'm actually pretty certain there is a way. The thing with generics in Java is that they are a compile time feature, and the compiler can be persuaded to do a great many things using the dark arts (a.k.a unsafe casts). Something that's been on my TODO list for the past year or so is to start prototyping a V2 API with most generics removed, and derive the V1 API from that using (among other things) annotations, Spoon itself and liberal sprinkles of the dark arts. Essentially, the V2 API would be the "real deal", and the V1 API wrapper around the V2 API with retained generics. Some hiccups might exist, such as where there's nasty use of generics in return types, which may necessitate some minor breakage to clean up.

Finding the time to get around to this has proven to be a little bit on the tricky side of things, though. But I figure I'll get there eventually.

Oh yeah using a wrapper it's definitely possible, but it's kind of hard to maintain in my opinion.

[Auties00]

is it mandatory for all public methods to remain there? I have kept them all, but deprecated a few and I'm not sure if that's what I'm supposed to do

Basically, see this section in the CONTRIBUTING.md. If the methods are internal, you can remove them, but for anything else deprecating is the preferred way. I think everything else depends on the actual method and what alternatives are feasible.

I've finished the feature btw, I'm working on making all the tests pass with no performance problems