feat: allow spoon to support multi-class definitions #2772
Conversation
| spoon.addInputResource("src/test/resources/multiclass/module2"); | ||
| spoon.getEnvironment().setAllowMutliTypeDefinition(true); | ||
| spoon.buildModel(); | ||
| assertNotNull(spoon.getFactory().Class().get("A")); |
There was a problem hiding this comment.
Which one do you get and which one do you expect?
There was a problem hiding this comment.
I have no way to know, it is jdt that handles it
|
API changes: 2 (Detected by Revapi) Old API: fr.inria.gforge.spoon:spoon-core:jar:7.2.0-20181120.234134-99 / New API: fr.inria.gforge.spoon:spoon-core:jar:7.2.0-SNAPSHOT
|
|
|
||
| @Override | ||
| public void setAllowMultiTypeDefinitions(boolean allowMultiTypeDefinitions) { | ||
| this.allowMultiTypeDefinitions = allowMultiTypeDefinitions; |
There was a problem hiding this comment.
shouldn't this mode associated with noclasspath by default?
Because if you're using it, basically according to your test you will ignore one of the declaration. That means you might do getDeclaration() on an element which is not available in the decl since it's not necessarily the right declaration.
IMO noclasspath will make Spoon more tolerant with such problems. WDYT?
There was a problem hiding this comment.
No it does not work like that since it is the same signature the model should correct (at least in the type level).
It is basically the same behaviour as the runtime environment of java. When the same class is declared several times only one is loaded is considered (and depends on the order of classpath).
It is not perfect because people do dirty stuff that but it is unrelated to noclasspath.
There was a problem hiding this comment.
the signature of the class is the same, but inside the class you can have different members, can't you?
Like in your example, what happens if you have one A with method foo() and another with bar() and let's say you want to spoon the model to check invocations of bar()?
There was a problem hiding this comment.
Do you realize that this would completely break the execution of the application?
There was a problem hiding this comment.
pretty much like when you don't load the right logger implementation yes. :)
I don't know your usecase here, maybe you're trying to load multiple version of the same project with small differences between them, so it'll be wrong in that case.
There was a problem hiding this comment.
I'm just trying to spoon some multi-modules projects (maven, gradle, ant multi modules). ANd yes you can declare several times the same classes.
It also happens with generated-sources. It is an unknown usecase but it does not seem that rare
There was a problem hiding this comment.
ok. I think a better name can be good for the option then: I misunderstood its goals at first :)
Something like allowIdenticalDeclarations? WDYT?
|
LGTM. Will be super useful for large scale automated analysis of big repos. |
That is the core problem of this PR. I think it is not enough to let JDT to produce two models of class with name A and put them somehow into one position in the model. I think we have to change API of Spoon to be able If I understand use case well there is possible to have internal invisible classes in modules. Because they are internal and invisible to others it is also possible that even if they have the same qualified name, these classes are completely different and independent. WDYT? Do I understand that use case well? |
According to @tdurieux comment here #2772 (comment) he's talking about a usecase where the class are exactly the same. That's why I suggested to change the name of the option to avoid confusion. |
|
So then this PR should include some check that both classes are 100% equal ... to avoid confusion in case I was speaking about. |
|
JDT does not really create two model of the class. The binding of one of them is null it means that it just created the ast of it but never considered it in the model.
And when there are not equals what do we do? |
I do not know your use case well. I just suggest to early warn about the situation when parsed sources contains two different classes with same unique name. So may be we should throw an exception. |
|
I don't really like the idea of throwing an exception because there is no workaround. |
|
+1 for the exception
if the case occurs, it will most certainly return wrong results for the user and it will be a real pain to debug. Now what we might do to help this case is to provide a list of source files to ignore in the environment. Then the user on the first run will get an exception with the full path of the conflicting file, and will be able to properly ignore it by adding a new argument. |
and it occurs. See first comment of #2761 ... where one class was, by configuration mistake, found twice. |
|
That is why I put an option and not activated it by default |
|
Is the JDK compiler configured well to compile Java9 modules? It seems to me strange that it cannot compile and resolve dependencies of each module individually. Is the situation same if you extended the test case and add real module definition, which marks the package with A class as internal? |
|
This feature is to support maven modules (not Java 9 modules), the compiler has no way to do the difference between each class. |
|
ok, then forgot all my comments. It is then different situation. |
|
@surli do you merge? |
|
As I suggested I'd like that we change the name of the option: for me it's ambiguous regarding the real usecase. I proposed to use |
|
I'm not a big fan of |
ok for me |
|
I just rename the method |
|
Thanks @tdurieux ! |
fix #2770