groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jochen Theodorou <>
Subject Re: Towards a better compiler
Date Sun, 23 Apr 2017 16:15:36 GMT
On 23.04.2017 17:08, Cédric Champeau wrote:
> Any tool that performs bytecode analysis, or computes signatures is
> affected. So if the compiler is not reproducible, since your at the
> bottom of the execution, you affect all tools that are built on top.
> This affects Gradle, but also Grails, IntelliJ (because different
> bytecode forces reindexing), tools which perform bytecode decoration
> (intercepting Foo#foo() is not the same as intercepting Bar#foo()) so it
> also affects correctness.

I don´t 100% agree, but let´s leave it at that.

>     Also I doubt we can be 100% reproducible as long as we use
>     reflection somewhere, simple because reflection is not stable. Only
>     because of the great contribution from intellij to be able read
>     class files directly without reflection in the compiler we have the
>     ability for *most* classes to have a stable result depending on the
>     occurrence in bytecode. But for for example the Groovy classes
>     directly known by the compiler like Closure, this won´t be the case.
>     100% stability is not achievable simply by using linked maps and sets.
> I agree, but I don't get the point of not trying to do better.

Didn´t want to say that. I would like to have an achievable and specific 

> It should
> be pretty obvious that for the same sources and same compile classpath,
> we should produce the same output. I don't get why we wouldn't try to
> fix this, even incrementally, as we found bugs. This is better.

because that is no different "towards a better", than what we did 
before. Ok, maybe I misunderstood and the original mail´s intention.

> I agree
> that we cannot do 100% reproducibility today, because we don't implement
> equals/hashcode, which means we're system dependent (and probably JDK
> dependent too).

Implementing equals/hashcode does not give you any order in a Set or Map 
you can rely on. Really, I still don´t get what you target with that. 
You require them in set to avoid duplicates.

So if you give back a Set<ClassNode>, then it is potentially wrong to 
use a Set as long has ClassNode does not implement equals and hashcode 
*or* this Set should actually be a List - unless of course you can 
ensure two supposed to be equal ClassNodes are not used here, but then 
again the Set should be a List. And I also think it is no solution to 
internally use a LinkedHashSet, and externally use a Set. If the order 
is a given and guaranteed element of your API, then it should be 
reflected in the type and LinkedHashSet should be used for the parameter 
or return type.

Of course the same goes for the usage of MethodNode as set element.

And if we talk about Maps, then the same should be said about the Map 
type at least, because I hope we do not use AST elements as keys anywhere.

This means for me that at least the fix you did for getAllInterfaces is 
not sufficient, since this solution still allows for duplicates. And I 
don´t think ClassNode should have an equals and hashcode either, not 
before with the twin role of being a reference to a type or the type itself.

> And implementing those is doing to be hard, because our
> structures are not immutable (so mutating something that is already in
> the set/map is problematic).

then don´t. Let´s change the API to not return Set.

> Anyway, I'm not asking for 100%
> reproducible today. I'm asking for every dev to realize that we have a
> problem that leads to non deterministic behavior, and that everyone
> should try to take that into consideration when writing code, and
> futhermore, tests.

fair enough...only I would add that this should be also taken into 
consideration when doing reviews.

>     once you include reflection here... Those cases you could solve by
>     sorting though.
> Yes, we should avoid reflection as much as possible. I wonder if there
> are still lots of places in the compiler where we do this: reflection
> means loading the classes and too often initializing, which is probably
> no good in any case.

We do not initialize the classes if we must not, which is only the case 
for annotations and annotations using constants - if reflection is used 
for them. Reflection is our fallback. For example the 
GroovyClassLoader/GroovyShell/Eval/GroovyScriptEngine/basically any 
runtime compilation logic will in most cases reflection. And you won´t 
have a transform path or compile class path there either. 
GroovyScriptEngine could possibly changed here somewhat... but I don´t 
see that for the more general cases of GCL, shell and eval.

>         Especially, order of interfaces in
>         declaration type matter (and they are reproducible today), We
>         *must not*
>         reorder them, or it would change semantics (typically for traits). I
>         have fixed the bugs we, in the Gradle team, have identified, but my
>         email was there to mention that we should take better care of this,
>         because we do a pretty bad job at checking that the behavior of the
>         compiler is deterministic.
>     well, that was no real option for a long time and no target till
>     now. But if you say your fixes are enough for gradle, then your
>     tests should ensure this stays. And then we don´t do a bad job at it
>     anymore, do we?
> I have added tests for the cases I fixed, yes. They are not
> cross-platform, though.

well, I do think the fixes are not sufficient, but I think for other 
reasons than you do ;)

>         Regarding AST transformations classpath, I had actually
>         forgotten that
>         Gradle integrates directly with the compiler, so can use the 2
>         different
>         "classpath". But AFAIK, our CLI doesn't. This should be easy to fix.
>     easy fix? depends... first of all... what will you gain from that?
>     Since I have no idea where you get an advantage from those things
>     outside a build tool that goes beyond something like make and ant, I
>     fail to see the advantage
> There would be an advantage for the Groovy compiler itself: today, if
> you use `groovyc`, you put everything on the compile classpath. We don't
> make the difference between classes that need to be found for
> implementation of the compiled classes, and those which are dependencies
> of AST transformations. The consequence is the same as the one people
> have with annotation processors when on the compile classpath instead of
> the annotation processor path: mixing classpath which have nothing to do
> together. In particular, say the user code requires Guava 19, but an AST
> transformation was built using Guava 17. Then you have a conflict, but
> the user shouldn't care about this, because Guava 17 is an
> implementation dependency of an AST transformation: nothing that should
> prevent them from using the class they wish, because once compiled,
> Guava 17 is not required anymore. It's about better modelling of
> dependencies, which has direct consequences on user pain.

ok, yes, I agree with that one. I still think for simplicity the current 
behaviour has to be the default.

bye Jochen

View raw message