These are chat archives for ceylon/ceylon-ide-eclipse

6th
May 2016
Roland Tepp
@luolong
May 06 2016 07:31
hum .. it might have been some other mixup … Yesterday at least my project had all the autocompletions. A collegue who I encouraged to develop a small internal service in Ceylon grumbled and I remembered that I had similar problem at some point
any way, I suggested he try out newer dev builds and he said, thet the dev builds page on ceylon-lang.org points at even older build (v1.2.0)
Maybe we should point it to nightly builds from RH Jenkins instead?
Gavin King
@gavinking
May 06 2016 12:44
@davidfestal do we really need the whole “rebuild with Ceylon classes” thing
if we’re not really supporting circular refs?
could we let you turn that off?
David Festal
@davidfestal
May 06 2016 12:45
it's not related to circular refs
Gavin King
@gavinking
May 06 2016 12:45
i mean deps from Jav to Ceylon
David Festal
@davidfestal
May 06 2016 12:45
related to bi-directional interop
OK, sorry
Gavin King
@gavinking
May 06 2016 12:45
we don’t really support it, AFAICT
David Festal
@davidfestal
May 06 2016 12:45
we support it partly
Gavin King
@gavinking
May 06 2016 12:45
it kinda works, sometimes
David Festal
@davidfestal
May 06 2016 12:46
and use it in the IDE Eclipse Plugin project
Gavin King
@gavinking
May 06 2016 12:46
him ok
David Festal
@davidfestal
May 06 2016 12:46
but yeah, that could be an option
I agree
by the way in IntelliJ we have already thought of an alternate way to see Ceylon structure from Java code
that will not require having the Ceylon classes generated
that will be better in the end
Bastien Jansen
@bjansen
May 06 2016 12:47
but harder to maintain ;)
David Festal
@davidfestal
May 06 2016 12:47
but in Eclipse it won't be available since JDT is not easily extendable
not sure
So yes, in Eclipse we could disable it
but keep in mind that the build abstraction is still not migrated in the Eclipse plugin
I already add it to IntelliJ
for evident timing reasons
and will do it, along with proper tests, in Eclipse afterwards
this will make things much cleaner than what we have today
(CeylonBuilder)
that should have been refactored for years !
Gavin King
@gavinking
May 06 2016 13:03
sure
Bastien Jansen
@bjansen
May 06 2016 13:25
Maybe we can target the new build for 1.3 instead of 1.2.3 in eclipse
This way we can test it in intellij, and not break things in a minor release
David Festal
@davidfestal
May 06 2016 13:26
remember that releasing a 1.2.3 IDE (=> backporting changes on the non-abstracted branch) takes much time also
so, dont know honestly
Bastien Jansen
@bjansen
May 06 2016 13:26
Ah right
Do we have the new completion and all that stuff in 1.2.2?
David Festal
@davidfestal
May 06 2016 13:27
don't remember exactly
but quite a number of things are old
and so won't support back-porting recent changes enhancements
Bastien Jansen
@bjansen
May 06 2016 13:27
It's time we finally get rid of that branch
David Festal
@davidfestal
May 06 2016 13:27
I find also
but this requires somemore efforts (mainly from me) to come to a consistent state with both IDEs
Bastien Jansen
@bjansen
May 06 2016 13:28
We have been using the features on master without any problems for months now
David Festal
@davidfestal
May 06 2016 13:28
yeah
right
only the build abstraction must be reintegrated into Eclipse and it's OK
Bastien Jansen
@bjansen
May 06 2016 13:28
Well you could still release the eclipse plugin from master, but without the new build
David Festal
@davidfestal
May 06 2016 13:28
in fact I've designed things so that the Eclipse side can keep the same structure first
Bastien Jansen
@bjansen
May 06 2016 13:28
And postpone that to the next version
David Festal
@davidfestal
May 06 2016 13:29
Ah, why not
that's a possibility
Bastien Jansen
@bjansen
May 06 2016 13:29
And when we see that the new build works, and you have time to integrate it, then we'll add it to eclipse
David Festal
@davidfestal
May 06 2016 13:30
That's possibl like that also
Gavin King
@gavinking
May 06 2016 17:38
wtf
shared actual default ModifiablePhasedUnitAlias? phasedUnit =>
        unsafeCast<ModifiablePhasedUnitAlias?>(super.phasedUnit);
are you guys really using this unsafeCast() function to work around design issues?!
I think i will delete that function and make you guys fix all the breakage that results
used in 109 (!!) places!
David Festal
@davidfestal
May 06 2016 17:39
no no please
Gavin King
@gavinking
May 06 2016 17:39
please stop
that simply can’t be right
David Festal
@davidfestal
May 06 2016 17:40
these are used exaclty in places we know for sure that the result is this type
as a replacement for assert
Gavin King
@gavinking
May 06 2016 17:40
so use assert then
David Festal
@davidfestal
May 06 2016 17:40
in case of types with type parameters
that would call isReified under the hood
Gavin King
@gavinking
May 06 2016 17:40
why?
David Festal
@davidfestal
May 06 2016 17:40
and would be much less performant
Gavin King
@gavinking
May 06 2016 17:41
in particular, the code i just showed does not need a typecast
David Festal
@davidfestal
May 06 2016 17:41
it's mainly a performance issue
why ?
Gavin King
@gavinking
May 06 2016 17:41
well it should perform the same, shouldn’t it?
you’re not narrowing the type parameters!
David Festal
@davidfestal
May 06 2016 17:41
it calls isReified whic is long
ModifiablePhasedUnitAlias is an alias to ModifiablePhasedUnit with 4 type parameters
Gavin King
@gavinking
May 06 2016 17:42
if it is slow then optimize it
David Festal
@davidfestal
May 06 2016 17:42
it's related to the metamodel
Gavin King
@gavinking
May 06 2016 17:42
since you’re only actually narrowing the outer type
David Festal
@davidfestal
May 06 2016 17:42
we all know this
I'm trying to make the IDE abstraction not too slw
slow
Gavin King
@gavinking
May 06 2016 17:43
there are many cases where this check is optimized away to an instanceof David
I think you’re taking guesses
instead of actually measuring stuff
David Festal
@davidfestal
May 06 2016 17:43
when the meta-model will be more performance, we'll be able to come back to asserts
I didn't have time to measure
Gavin King
@gavinking
May 06 2016 17:43
no, i think we should remove this rubbish
it’s not Ceylon
David Festal
@davidfestal
May 06 2016 17:44
please do this after I finish the abstraction
Gavin King
@gavinking
May 06 2016 17:44
it’s quasi-Java
David Festal
@davidfestal
May 06 2016 17:44
or I'm OK to take time to measure it afterwards
by replacing it with asserts
Gavin King
@gavinking
May 06 2016 17:45
i mean what crap code all this is
David Festal
@davidfestal
May 06 2016 17:45
wdym ?
Gavin King
@gavinking
May 06 2016 17:45
ModifiableSourceFile, IdePhasedUnit
accept objects in their constructors
David Festal
@davidfestal
May 06 2016 17:45
it's been existing for a while and you were OK with this
already in the Java version
Gavin King
@gavinking
May 06 2016 17:45
and don’t constrain them to be the subtype that you apparently expect
and then unsafeCast() then down
instead of just accepting the correct type in the constructor
David Festal
@davidfestal
May 06 2016 17:46
no no
Gavin King
@gavinking
May 06 2016 17:46
and returning that reference directly
this is nonense
don’t ever write code like that
David Festal
@davidfestal
May 06 2016 17:46
things are not so simple Gavin
Gavin King
@gavinking
May 06 2016 17:46
sure they are
shared new(
    BaseFileVirtualFile unitFile,
    BaseFolderVirtualFile srcDir,
    Tree.CompilationUnit cu,
    Package p,
    ModuleManager moduleManager,
    ModuleSourceMapper moduleSourceMapper,
    TypeChecker typeChecker,
    JList<CommonToken> tokenStream) extends PhasedUnit(unitFile, srcDir, cu, p, moduleManager, moduleSourceMapper, typeChecker.context, tokenStream) {
    typeCheckerRef = WeakReference<TypeChecker>(typeChecker);
}

shared new clone(PhasedUnit other) extends PhasedUnit(other) {
    if (is IdePhasedUnit other) {
        typeCheckerRef = WeakReference<TypeChecker>(other.typeChecker);
    }
}

shared actual default BaseIdeModuleSourceMapper moduleSourceMapper => 
        unsafeCast<BaseIdeModuleSourceMapper>(super.moduleSourceMapper);

shared actual default BaseFileVirtualFile unitFile =>
        unsafeCast<BaseFileVirtualFile>(super.unitFile);

shared actual default BaseFolderVirtualFile srcDir =>
        unsafeCast<BaseFolderVirtualFile>(super.srcDir);
David Festal
@davidfestal
May 06 2016 17:46
because at the root of the hierarchy there are Java classes that don't obey Ceylon design
Gavin King
@gavinking
May 06 2016 17:46
oh well, actually in this case it actually does constraint the inputs
so fine i can just remove the stupid casts
David Festal
@davidfestal
May 06 2016 17:47
Again, please can you let me finish integration this abstraction
?
surely it's not you would like to see it
but I'd like at least to finish it as a consistent thing
so please don't change things just in the middle of my integration phase :-(
And I promise I'll take some time to rework this as you like
but remember that there was a time some Ceylon abstracted code and the existing Java code was coexisting
and the Unit / PhasedUnit type hierarchies have been there for Years in the Java version
and you were OK with this design
So yes, this might look like Java code at some places
Gavin King
@gavinking
May 06 2016 17:50
what i’m saying is that these classes should remember the reference they were passed in their constructor
and return that from the refinements
instead of narrowing the supertype’s reference
David Festal
@davidfestal
May 06 2016 17:50
that's not always possible
Gavin King
@gavinking
May 06 2016 17:51
sure it is
David Festal
@davidfestal
May 06 2016 17:51
because in some cases the reference is not passed by the constructor
but created by a mecanism managed in an ancestor class
do you remember PhasedUnit.createUnit ?
Gavin King
@gavinking
May 06 2016 17:51
well that’s simply rubbish then
David Festal
@davidfestal
May 06 2016 17:51
this returns a Unit
Gavin King
@gavinking
May 06 2016 17:51
these things should be immutable
David Festal
@davidfestal
May 06 2016 17:52
That's related to how the Phased / Unit model is designed
and I inherited from this years ago
Gavin King
@gavinking
May 06 2016 17:52
not have some bizarre state model involving an inheirted mechanism that computes state
David Festal
@davidfestal
May 06 2016 17:52
Of course if we started the whole in Ceylon (typechecker, IDE, etc ) right now
we would do differently
yes
Gavin King
@gavinking
May 06 2016 17:53
look if this was in, say, 5 places
fine
David Festal
@davidfestal
May 06 2016 17:53
but that's not the case
Gavin King
@gavinking
May 06 2016 17:53
but it’s used in 109 places!!
that’s unacceptable
David Festal
@davidfestal
May 06 2016 17:53
Some of those places are only for performance
I'll have a look
but as for the ModuleSourceMapper, ModelLoader, etc ...
all these java components are quite intricated
and have references to each other
Gavin King
@gavinking
May 06 2016 17:55
why do you even have these nasty parellel class hierrchies
David Festal
@davidfestal
May 06 2016 17:55
and of course the derived classes I created had to do such asserts
Gavin King
@gavinking
May 06 2016 17:55
that;s a code smell to begin with
nothing should depend on BaseIdeModuleSourceMapper
David Festal
@davidfestal
May 06 2016 17:55
because I'm extending a some existing framework
Gavin King
@gavinking
May 06 2016 17:56
it’s a strategy
David Festal
@davidfestal
May 06 2016 17:56
wdym ?
Gavin King
@gavinking
May 06 2016 17:56
there should be only one sourcemapper api
why do things depend on BaseFileVirtualFile??
David Festal
@davidfestal
May 06 2016 17:57
The BaseXXXX classes / interfaces are skeleton of the complete class/interface but without type parameters
so that in many other places, I don't need to add type parameters to the using / instanciting scope
Gavin King
@gavinking
May 06 2016 17:58
use-site variance?
David Festal
@davidfestal
May 06 2016 17:58
And believe me, it was very useful when having both the old Java code and the new Ceylon code working together
Gavin King
@gavinking
May 06 2016 17:58
that’s what it’s for
Foo<out Anything, in Nothing>
you can write an alias for it
David Festal
@davidfestal
May 06 2016 17:59
might be
not sure this is OK for all cases
Bastien Jansen
@bjansen
May 06 2016 17:59
we already have a few aliases like that (AnyPhasedUnit etc)
David Festal
@davidfestal
May 06 2016 17:59
but might be for some cases
Gavin King
@gavinking
May 06 2016 18:00
look david, you have a huge profusion of subclasses of subclasses
with interrelations between parellel class hierarchies
David Festal
@davidfestal
May 06 2016 18:00
But again please open an issue with all the design problems you want me to fix
Gavin King
@gavinking
May 06 2016 18:00
and all of these subclasses are generic with 4-5 type parameters
something wrong there
David Festal
@davidfestal
May 06 2016 18:01
and I'll work on it as soon as I've minished my current work
the 4 parameters are OK
IMO
Gavin King
@gavinking
May 06 2016 18:01
whenever you arrive at a design like that
it’s time to take a step back and ask what problem you’re trying to solve with all that complexity
David Festal
@davidfestal
May 06 2016 18:02
Hey, these hierarchies were already there in the Java version
Gavin King
@gavinking
May 06 2016 18:02
I have never written a class with four type parameters in my whole career
David Festal
@davidfestal
May 06 2016 18:02
in Java ?
the thing is: you don't need to write them at all places
You don't need to write them if you don't need to use them
Gavin King
@gavinking
May 06 2016 18:03
well if 4 type parameters were ok, you wouldn’t have had to create BaseXxxxx, would you?
David Festal
@davidfestal
May 06 2016 18:04
I probably didn't need the BaseXXX if I had correctly used use-site variance
this is probably my fault here
OK, I probably can reduce the number of classes if I remove the Base classes
sure
I won't do it right now
of course
but I'm not against it
sure
as for the parallel hierarchies : they correspond to existing objects in the Ceylon tooling
Unit, VirtualFile, PhasedUnit
and the various managers that I have to extend
Gavin King
@gavinking
May 06 2016 18:07
but c’mon: different kinds of units have different kinds of SourceMappers?
that doesn’t pass the smell test
David Festal
@davidfestal
May 06 2016 18:07
no no
why do you say that
there's only one type of source mapper
Gavin King
@gavinking
May 06 2016 18:07
shared actual default BaseIdeModuleSourceMapper moduleSourceMapper => 
        unsafeCast<BaseIdeModuleSourceMapper>(super.moduleSourceMapper);
David Festal
@davidfestal
May 06 2016 18:07
CeylonSourceMapper
Gavin King
@gavinking
May 06 2016 18:08
IdePhasedUnit exposes BaseIdeModuleSourceMapper to clients
that looks like rubbish to me
David Festal
@davidfestal
May 06 2016 18:08
the BaseXXX are rubbish yes
I already agreed with this
it exposes this because it doesn't have any type parameter
Gavin King
@gavinking
May 06 2016 18:08
but why does it refine it?
David Festal
@davidfestal
May 06 2016 18:09
and I wasn't using use-site variance
Gavin King
@gavinking
May 06 2016 18:09
instead of just returning ModuleSourceMapper like the supertype
David Festal
@davidfestal
May 06 2016 18:10
because I kwo the moduleSourceMapper instances used in the context of the IDE are IdeModuleSourceMappers
because they have additional methods useful for the IDE
that the typechecker version doesn't have of course
Gavin King
@gavinking
May 06 2016 18:10
then you’re sticking different new functionality on something that is supposed to be a trivial strategy object
David Festal
@davidfestal
May 06 2016 18:11
yeah, not "new fonctionality" necessarily
wait a minute if you want a detailed explanation about this specific case :-)
Gavin King
@gavinking
May 06 2016 18:14
it;s used only in the constructor of SourceFile
David Festal
@davidfestal
May 06 2016 18:14
IdeModuleSourceMapper returns the associated CeylonProject
Gavin King
@gavinking
May 06 2016 18:14
then why not have a separate API for that
ProjectAware or whatever
David Festal
@davidfestal
May 06 2016 18:14
because the quantity of things to abstract was huge
Gavin King
@gavinking
May 06 2016 18:14
that seems to be something that IdePhasedUnit could reasonably define
David Festal
@davidfestal
May 06 2016 18:15
and the design is not perfect :-)
That's fully OK to me that many things can be cleaned
designed better
Gavin King
@gavinking
May 06 2016 18:15
right, but bad design makes the abstraction work go slower not faster
David Festal
@davidfestal
May 06 2016 18:15
abstraction is finished now
I mean in its har part
and I'm migrating to it
and tweaking the ast implementation part
all the things you said until now can be changed / cleaned afterwards
Gavin King
@gavinking
May 06 2016 18:16
well that’s good
but i really dislike all the mess surrounding phasedunits
David Festal
@davidfestal
May 06 2016 18:17
(mainly the removal of the BaseXXX classes to replace it by use-ste variance)
Gavin King
@gavinking
May 06 2016 18:17
i have to do dowcasting and all kinds of stuff
for stuff that i feel could perfectly well be abstracted by a supertype
David Festal
@davidfestal
May 06 2016 18:17
What do you have to do ?
in which case do you need to do downcasting ?
Gavin King
@gavinking
May 06 2016 18:19
i sometimes need to downcast to ModifiableSomethingOrOther
David Festal
@davidfestal
May 06 2016 18:19
Some things could be much nicer related to PhasedUnit with a refactoring of the PhasedUnit object itself
Gavin King
@gavinking
May 06 2016 18:19
surely
David Festal
@davidfestal
May 06 2016 18:19
in the Java code or in ceylon code ?
Gavin King
@gavinking
May 06 2016 18:19
PhasedUnit was never a very strong abstraction
both
David Festal
@davidfestal
May 06 2016 18:20
yeah
Gavin King
@gavinking
May 06 2016 18:20
look, i’m the great defender of inheritance
David Festal
@davidfestal
May 06 2016 18:20
well, It's going to be cleaner when Eclipse will be using the Build Abstraction also
Gavin King
@gavinking
May 06 2016 18:20
but even i don’t use it like this
David Festal
@davidfestal
May 06 2016 18:20
because
for now the Eclipse plugin is a mix of old and new code
which is part of the reason why you have to do such downcasting
if oyu get the unit from the CeylonProject, you directly get the Unit in the typethere are really
either ProjectPhasedUnit or ExternalPhasedUnit from a source module, etc ...
and you directly have the specific methods allowed for them
the thing is the the legacy Eclipse plugin Java code
uses the raw typechecker API in many places
and doesn't take benefit of this structuration I made
at least that's my POV
but please please open an issue in ceylon-ide-common to keep track of this
of the cleaning or design fixes you expect
I have to go for a while
Gavin King
@gavinking
May 06 2016 18:24
alright, well, back to work...
David Festal
@davidfestal
May 06 2016 18:25
by the way, the debug of the build inside Intellij is going on
Gavin King
@gavinking
May 06 2016 18:25
wdym?
it’s making progress?
David Festal
@davidfestal
May 06 2016 18:25
yeah making progress I was meaning :-)
ah, my bad english again !
Gavin King
@gavinking
May 06 2016 18:26
excellent
David Festal
@davidfestal
May 06 2016 18:26
gtg, I'll be back later
Gavin King
@gavinking
May 06 2016 18:26
that’s good