Closed PaulKlint closed 9 years ago
I think the problem here is that there is also an abstract type named Exp
. When that is introduced, it knocks the concrete type out of scope (along with itself, it's imported as well). You would normally then be able to access it using a qualified name, like demo::lang::Exp::Concrete::WithLayout::Syntax::Exp
, but you cannot use qualified names in the parens before the concrete pattern.
@mahills you are completely right. In fact all problems in the Exp
example disappear by separating the concrete sort (Exp
) and the abstract sort (AExp
) but then we can no longer use implode
. The troubling thing is that the Exp
example is one of the first examples you want to show to a new user, but the assessment is (unfortunately):
Exp
.The current dilemma is:
implode
can work due to these naming issues.We have written the type checker and the compiler mostly relying on concrete patterns, but we will occasionally want a bridge between concrete and abstract syntax.
Has anyone ideas for a solution?
@PaulKlint , at least for this it would work to be able to give a qualified name in the concrete pattern, but in general this is just painful because the types are separate. It probably works in the interpreter because the interpreter just does bizarre stuff with these scenarios, it is probably merging the two types and then picking the right one out when it needs to (but, on the other hand, potentially turning things to value in other places).
One solution may be to say that these are both the same type, and just keep track of whether a type can have productions, constructors, or both. I'm not sure what other problems that would cause, though.
@mahills Indeed, qualified names in concrete patterns are not the solution either.
I like you idea about merging constructors and productions in a single type since that eliminates the current confusion. Find it hard to oversee the implications of this.
This solution would imply that instead of
module demo::lang::Exp::Combined::Automatic::Load
import demo::lang::Exp::Combined::Automatic::Parse; /*1*/
import demo::lang::Exp::Abstract::Syntax; /*2*/
import ParseTree; /*3*/
public demo::lang::Exp::Abstract::Syntax::Exp load(str txt) =
implode(#demo::lang::Exp::Abstract::Syntax::Exp, parse(txt));
we can write
module demo::lang::Exp::Combined::Automatic::Load
import demo::lang::Exp::Combined::Automatic::Parse; /*1*/
import demo::lang::Exp::Abstract::Syntax; /*2*/
import ParseTree; /*3*/
public Exp load(str txt) =
implode(#Exp, parse(txt));
and that looks great!
I'm not sure it has any problems, I just want to be careful. It would definitely simplify things.
On Feb 11, 2014, at 5:43 PM, "Paul Klint" notifications@github.com<mailto:notifications@github.com> wrote:
@mahillshttps://github.com/mahills Indeed, qualified names in concrete patterns are not the solution either.
I like you idea about merging constructors and productions in a single type since that eliminates the current confusion. Find it hard to oversee the implications of this.
— Reply to this email directly or view it on GitHubhttps://github.com/cwi-swat/rascal/issues/493#issuecomment-34817654.
Well, the subtyping of concrete nonterminal types with Tree
and of algebraic datatypes with node
could be interesting to handle (since Tree
is also a subtype of node
, especially).
We have considered such option before and did not arrive at a good design yet. Indeed the Tree type is in the way, but also the fact that a parse tree is really different from an ast. We also need a common super type of all parse trees for language independent processing...— Jurgen J. Vinju CWI SWAT INRIA Lille UvA master software engineering http://jurgen.vinju.org
On Wed, Feb 12, 2014 at 4:39 PM, mahills notifications@github.com wrote:
Well, the subtyping of concrete nonterminal types with
Tree
and of algebraic datatypes withnode
could be interesting to handle (sinceTree
is also a subtype ofnode
, especially).Reply to this email directly or view it on GitHub: https://github.com/cwi-swat/rascal/issues/493#issuecomment-34880552
Also there would be type safety problems where asts could suddenly be nested inside parse trees. — Jurgen J. Vinju CWI SWAT INRIA Lille UvA master software engineering http://jurgen.vinju.org
On Wed, Feb 12, 2014 at 6:17 PM, Jurgen J. Vinju notifications@github.com wrote:
We have considered such option before and did not arrive at a good design yet. Indeed the Tree type is in the way, but also the fact that a parse tree is really different from an ast. We also need a common super type of all parse trees for language independent processing...— Jurgen J. Vinju CWI SWAT INRIA Lille UvA master software engineering http://jurgen.vinju.org On Wed, Feb 12, 2014 at 4:39 PM, mahills notifications@github.com wrote:
Well, the subtyping of concrete nonterminal types with
Tree
and of algebraic datatypes withnode
could be interesting to handle (sinceTree
is also a subtype ofnode
, especially).Reply to this email directly or view it on GitHub:
https://github.com/cwi-swat/rascal/issues/493#issuecomment-34880552
Reply to this email directly or view it on GitHub: https://github.com/cwi-swat/rascal/issues/493#issuecomment-34891870
Ah, good point, we really can't keep track of whether the type was constructed based on a constructor or a production.
swatbot mailto:notifications@github.com February 12, 2014 at 12:20 PM Also there would be type safety problems where asts could suddenly be nested inside parse trees. — Jurgen J. Vinju CWI SWAT INRIA Lille UvA master software engineering http://jurgen.vinju.org
On Wed, Feb 12, 2014 at 6:17 PM, Jurgen J. Vinju notifications@github.com wrote:
We have considered such option before and did not arrive at a good design yet. Indeed the Tree type is in the way, but also the fact that a parse tree is really different from an ast. We also need a common super type of all parse trees for language independent processing...— Jurgen J. Vinju CWI SWAT INRIA Lille UvA master software engineering http://jurgen.vinju.org On Wed, Feb 12, 2014 at 4:39 PM, mahills notifications@github.com wrote:
Well, the subtyping of concrete nonterminal types with
Tree
and of algebraic datatypes withnode
could be interesting to handle(since
Tree
is also a subtype ofnode
, especially).Reply to this email directly or view it on GitHub:
https://github.com/cwi-swat/rascal/issues/493#issuecomment-34880552
Reply to this email directly or view it on GitHub: https://github.com/cwi-swat/rascal/issues/493#issuecomment-34891870
— Reply to this email directly or view it on GitHub https://github.com/cwi-swat/rascal/issues/493#issuecomment-34892307.
Paul Klint mailto:notifications@github.com February 11, 2014 at 4:36 AM
Compiling |demo::lang::Exp::Combined::Manual::Load|:
|module demo::lang::Exp::Combined::Manual::Load
import demo::lang::Exp::Concrete::WithLayout::Syntax; /1/ import demo::lang::Exp::Abstract::Syntax; /2/ import demo::lang::Exp::Combined::Manual::Parse; /3/ import String;
public Exp load(str txt) = load(parse(txt)); /4/
public Exp load((Exp)
<IntegerLiteral l>
) /5/ = con(toInt("")); public Exp load((Exp) <Exp e1> * <Exp e2>
) = mul(load(e1), load(e2)); public Exp load((Exp)<Exp e1> + <Exp e2>
) = add(load(e1), load(e2)); public Exp load((Exp)( <Exp e> )
) = load(e); |leads to a lot of name confusion:
error("Multiple functions found which could be applied", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (616,7,<17,9>,<17,16>)) error("Syntax type Exp is not defined", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (508,3,<14,17>,<14,20>)) error("Syntax type Exp is not defined", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (444,3,<12,34>,<12,37>)) error("Multiple functions found which could be applied", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (468,8,<13,13>,<13,21>)) error("Multiple functions found which could be applied", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (548,8,<15,13>,<15,21>)) error("Syntax type Exp is not defined", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (525,3,<14,34>,<14,37>)) error("Syntax type Exp is not defined", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (433,3,<12,23>,<12,26>)) error("Type Exp not declared", project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Parse.rsc (128,3,<5,7>,<5,10>)) error("Syntax type Exp is not defined", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (514,3,<14,23>,<14,26>)) error("Multiple functions found which could be applied", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (478,8,<13,23>,<13,31>)) error("Multiple functions found which could be applied", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (558,8,<15,23>,<15,31>)) error("Syntax type Exp is not defined", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (331,3,<10,17>,<10,20>)) error("Syntax type Exp is not defined", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (427,3,<12,17>,<12,20>)) error("Unexpected type: type of body expression, Exp, must be a subtype of the function return type, fail", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (384,17,<11,9>,<11,26>)) error("Syntax type Exp is not defined", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (587,3,<16,17>,<16,20>)) error("Syntax type Exp is not defined", rascal:///demo/lang/Exp/Combined/Manual/Load.rsc (595,3,<16,25>,<16,28>)) — Reply to this email directly or view it on GitHub https://github.com/cwi-swat/rascal/issues/493.
I am further exploring this issue and a bit simpler example is this:
module experiments::Compiler::Examples::ExpTst
import ParseTree;
import demo::lang::Exp::Concrete::WithLayout::Syntax;
import demo::lang::Exp::Abstract::Syntax;
demo::lang::Exp::Concrete::WithLayout::Syntax::Exp v = (Exp) `1`;
which gives error("Syntax type Exp is not defined",|rascal:///experiments/Compiler/Examples/ExpTst.rsc|(220,3,<7,56>,<7,59>))
@mahills you said that "the abstract type knocks the concrete type out of scope", but would it in any way be possible to use the knocked out type in the case of (Exp)
that prefixes a concrete pattern or term?
I'm looking at this again, so it's probably best we restart the conversation on this topic :)
Honestly, I'm still not sure the best approach here. The problem remains as before, even after the various changes -- because the current module would import two distinct types named Exp
, one ADT and one nonterminal, Exp
is not added into scope, but instead the longer, qualified names would be used. It may be possible to maintain a separate scope just for uses where we can distinguish between concrete and abstract types, like we have above (here, we would know that Exp
has to be concrete, and can react accordingly), but my concern would be that this could lead to confusion. It may be hard to explain why, in some places, you can say Exp
, but in others, even in the same statement, you cannot.
We need a radical solution here, i.e. A language change. I believe a type constructor to separate the two name spaces would help or a different solution for implode which does not require the names to be equal or both..— Jurgen J. Vinju CWI SWAT INRIA Lille UvA master software engineering http://jurgen.vinju.org
On Wed, Jun 18, 2014 at 6:07 PM, mahills notifications@github.com wrote:
I'm looking at this again, so it's probably best we restart the conversation on this topic :)
Honestly, I'm still not sure the best approach here. The problem remains as before, even after the various changes -- because the current module would import two distinct types named
Exp
, one ADT and one nonterminal,Exp
is not added into scope, but instead the longer, qualified names would be used. It may be possible to maintain a separate scope just for uses where we can distinguish between concrete and abstract types, like we have above (here, we would know thatExp
has to be concrete, and can react accordingly), but my concern would be that this could lead to confusion. It may be hard to explain why, in some places, you can sayExp
, but in others, even in the same statement, you cannot.Reply to this email directly or view it on GitHub: https://github.com/cwi-swat/rascal/issues/493#issuecomment-46456638
I agree. Given code like this:
Exp myfun(Exp e) = ...
where Exp
has been imported both as an ADT and as a nonterminal, this really is ambiguous -- not only can the checker not determine which of the two should be used here, a human reading this code may not be able to figure this out either (except, maybe, for whoever wrote it).
@mahills I agree that the current situation is unacceptable and that we need something better. I will discuss this the coming days with @jurgenvinju and @tvdstorm and maybe we can have a Skype session after that. Any ideas/suggestions appreciated.
For now such ambiguity should be flagged as an error and resolvable using qualification. Is that possible without a big effort?— Jurgen J. Vinju CWI SWAT INRIA Lille UvA master software engineering http://jurgen.vinju.org
On Wed, Jun 18, 2014 at 6:54 PM, mahills notifications@github.com wrote:
I agree. Given code like this: Exp myfun(Exp e) = ...
where
Exp
has been imported both as an ADT and as a nonterminal, this really is ambiguous -- not only can the checker not determine which of the two should be used here, a human reading this code may not be able to figure this out either (except, maybe, for whoever wrote it).Reply to this email directly or view it on GitHub: https://github.com/cwi-swat/rascal/issues/493#issuecomment-46462475
We can flag it as an error, I'll just need to carry around information about which names were not added because they would have created ambiguities, like Exp
in this example. It is already possible to use the qualified versions of the names, at least in the checker -- I'm not sure if all the positions in the program above allow fully qualified names, some may only allow unqualified names. I'll have to check that later.
Not sure it was mentioned yet, but the current solution is to keep both ADT and grammar in separate modules, and use qualified names only in a custom Implode module. The other unfortunate thing is is that qualified non-terminal names don't work in start[X]
.
start[X] could be an exception because X can only be a non-terminal.
On Thu, Jun 19, 2014 at 1:25 PM, mahills notifications@github.com wrote:
We can flag it as an error, I'll just need to carry around information about which names were not added because they would have created ambiguities, like Exp in this example. It is already possible to use the qualified versions of the names, at least in the checker -- I'm not sure if all the positions in the program above allow fully qualified names, some may only allow unqualified names. I'll have to check that later.
— Reply to this email directly or view it on GitHub https://github.com/cwi-swat/rascal/issues/493#issuecomment-46549295.
Jurgen Vinju
Universiteit van Amsterdam
www: http://jurgen.vinju.org, http://www.rascal-mpl.nl, http://twitter.com/jurgenvinju skype: jurgen.vinju
@tvdstorm I thought that was the case, but wanted to check the grammar first to make sure my memory was accurate.
@jurgenvinju we can do that in multiple places (we could say the same of the use of the names inside concrete syntax patterns, for instance), but that would still leave open the question of how we differentiate a concrete Exp
from an abstract Exp
in function parameters, function return types, etc. I can always keep around separate namespaces inside the checker (a concrete namespace for cases where only a concrete type can be used, an abstract namespace for cases where only an abstract type can be used, plus the current combined namespace) but this is making the concept of scope for Rascal more complex.
Agreed. We have a simple concept for the other names and conflicts, namely merging the functions, datatypes, whenever possible, but we don't know how to merge abstract and concrete syntax yet.. A solution could be in this direction, where we truly consider data A and syntax A to produce concrete terms of the same abstract sort.
From the perspective of the liskov principle, embedding a concrete tree in an abstract one is fine, but vice versa is a problem. We need a step that automatically lifts an abstract tree to a concrete one to fix this.... Then we would always generate type safe trees and the abstract type and the syntax type could be unified. — Jurgen J. Vinju CWI SWAT INRIA Lille UvA master software engineering http://jurgen.vinju.org
On Thu, Jun 19, 2014 at 3:48 PM, mahills notifications@github.com wrote:
@tvdstorm I thought that was the case, but wanted to check the grammar first to make sure my memory was accurate.
@jurgenvinju we can do that in multiple places (we could say the same of the use of the names inside concrete syntax patterns, for instance), but that would still leave open the question of how we differentiate a concrete
Exp
from an abstractExp
in function parameters, function return types, etc. I can always keep around separate namespaces inside the checker (a concrete namespace for cases where only a concrete type can be used, an abstract namespace for cases where only an abstract type can be used, plus the current combined namespace) but this is making the concept of scope for Rascal more complex.Reply to this email directly or view it on GitHub: https://github.com/cwi-swat/rascal/issues/493#issuecomment-46561994
This file is still pestering us, @mahills what are your current thoughts on this?
Before delving into language changes (as suggested above and in the related issues) we need to get this one up and running.
Currently the messages are:
error("Syntax type Exp is not defined",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(422,3,<11,52>,<11,55>))
error("Syntax type Exp is not defined",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(669,3,<15,52>,<15,55>))
error("Syntax type Exp is not defined",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(553,3,<13,52>,<13,55>))
error("Syntax type Exp is not defined",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(559,3,<13,58>,<13,61>))
error("Syntax type Exp is not defined",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(675,3,<15,58>,<15,61>))
error("Syntax type Exp is not defined",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(791,3,<17,60>,<17,63>))
error("Syntax type Exp is not defined",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(783,3,<17,52>,<17,55>))
error("Syntax type Exp is not defined",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(686,3,<15,69>,<15,72>))
error("Syntax type Exp is not defined",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(570,3,<13,69>,<13,72>))
error("Function of type fun Exp(Exp) cannot be called with argument types (Tree)",|project://rascal/src/org/rascalmpl/library/demo/lang/Exp/Combined/Manual/Load.rsc|(330,19,<9,65>,<9,84>))
I now have this as the message in all the places where Syntax type Exp is not defined
was being shown:
Nonterminal Exp was not imported, use one of the following fully qualified type names instead: demo::lang::Exp::Concrete::WithLayout::Syntax::Exp,demo::lang::Exp::Abstract::Syntax::Exp
This could also do with some work (since it gives names for both concrete and abstract types), but at least it's more informative. It also gives a better description of the error -- Exp
wasn't imported as an unqualified name since there are two incompatible versions.
That is certainly an improvement.
We keep running in circles since, sadly, all roads to a solution are blocked by the fact that we can currently not use qualified names in certain places:
<demo::lang::Exp::Concrete::WithLayout::Syntax::Exp e2>
.(demo::lang::Exp::Concrete::WithLayout::Syntax::Exp) 1
start[X]
I propose therefore that we put this issue on hold and reenact it when we become more liberal in the use of qualified names.
In the mean time I will rewrite/delete some of the demo that cause these problems.
I'll see what I can do to handle those in the checker as well -- I may be able to handle all those cases without too much trouble.
But this will also require changes to the Rascal syntax and I am reluctant to do that now since that will break things in various places.
@PaulKlint for the three cases you mention the semantics is unambiguous; it can not be anything but a concrete syntax type. I suspect @mahills is planning to hard code this fact into the type checker as an intermediate solution.
@jurgenvinju something like that -- I'm just trying to decide if I should look them up manually, using the information I already have, or add another map to the configuration (which would be slightly faster but would take up even more space, then, and require maintenance to keep it up to date)
@mahills I guess you answered that one. Look up manually :+1:
@PaulKlint I believe all 3 cases are now supported
Super, will check this in a moment.
But on second thoughts, how could I test this? Any example will be syntactically incorrect!
Do you have an example where this works? Or should we just postpone testing this until we have syntactic support for the above 3 cases?
What I've done is to allow cases like the above to work with unqualified names. For instance, if you import a concrete and an abstract version of Exp
, Exp
will not be added to the type environment, but the qualified versions are available. If you use Exp
in a case where it can only be a sort, the code is first looking it up as it was before, but then is falling back to checking that Exp
was actually imported, wasn't added because of a name clash, and is in fact the name of a sort. In those cases, it is resolved anyway, since that is not ambiguous.
All the checking also makes sure it was really imported, instead of somehow "leaking" through (since we bring in all the grammar info transitively anyway).
Great! @PaulKlint so we need a test where Exp
is imported twice once as data and once as syntax, and used in a start[Exp]
but does not lead to a type check error.
The module that started this issue is a perfect example, since it does this (except for using start[Exp]
).
Indeed, the above example (with disambiguated names) now works fine (I was under the assumption that more disambiguation was needed). I am inclined to close this issue for now.
We end up with the following, quite ugly, but working code. I may remove this from the demos to avoid confronting Rascal novices with this.
module demo::lang::Exp::Combined::Manual::Load
import demo::lang::Exp::Concrete::WithLayout::Syntax; /*1*/
import demo::lang::Exp::Abstract::Syntax; /*2*/
import demo::lang::Exp::Combined::Manual::Parse; /*3*/
import String;
public demo::lang::Exp::Abstract::Syntax::Exp loadExp(str txt) = load(parseExp(txt)); /*4*/
public demo::lang::Exp::Abstract::Syntax::Exp load((Exp)`<IntegerLiteral l>`) /*5*/
= con(toInt("<l>"));
public demo::lang::Exp::Abstract::Syntax::Exp load((Exp)`<Exp e1> * <Exp e2>`)
= mul(load(e1), load(e2));
public demo::lang::Exp::Abstract::Syntax::Exp load((Exp)`<Exp e1> + <Exp e2>`)
= add(load(e1), load(e2));
public demo::lang::Exp::Abstract::Syntax::Exp load((Exp)`( <Exp e> )`)
= load(e);
One way to make the code look better is to choose different names for Exp
. Is @tvdstorm already finished with the new implode such that we can implode to different data
?
Actually, in this specific example we do the mapping manually and could rename as we like.
However, to be compatible with the implode
-based solution the names are the same here. I don't think the new implode
is already ready for prime time.
I guess we could also use aliases -- I haven't tried it here, but think it would work.
certainly aliases should work as well to make the code look better.
Right, then maybe add
alias AbstractExp = demo::lang::Exp::Abstract::Syntax::Exp;
and then use that for the return types, which would leave it well-typed while making it look better. That would at least take care of it for now. I'm happy to make the changes in a bit...
Thanks @mahills, but I have some other changes in mind as well. So will do this.
With 43 comments this is one of the longer discussions, nonetheless closing it now.
Compiling
demo::lang::Exp::Combined::Manual::Load
:leads to a lot of name confusion: