Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vals shadow type aliases #199

Closed
mpilquist opened this issue Aug 26, 2015 · 21 comments
Closed

Vals shadow type aliases #199

mpilquist opened this issue Aug 26, 2015 · 21 comments
Labels

Comments

@mpilquist
Copy link

› ./amm
Loading...
Welcome to the Ammonite Repl 0.4.5
(Scala version 2.11.7 Java 1.8.0_25)
@ type Foo = Int
defined type Foo
@ val Foo = 1
Foo: Int = 1
@ 1: Foo
Compilation Failed
Main.scala:52: not found: type Foo
1: Foo
   ^
@
@Blaisorblade
Copy link
Contributor

And the other way around. Continuing the transcript:

@ type Foo = Int
defined type Foo
@ val Foo = 1
Foo: Int = 1
@ 1: Foo
Compilation Failed
Main.scala:36: not found: type Foo
1: Foo
   ^
@ type Foo = Int
defined type Foo
@ Foo
Compilation Failed
Main.scala:36: not found: value Foo
Foo
^
@ 1: Foo
res3: Foo = 1

@Blaisorblade
Copy link
Contributor

First hypothesis: Ammonite and Scala REPL differ in how they wrap naked expressions to turn them into compilation units.
I'd compare Evaluator.wrapCode in repl/src/main/scala/ammonite/repl/interp/Evaluator.scala with the equivalent in the Scala REPL; but Preprocess might also be relevant.

Alternatively, I'd use -Xprint:typer to debug what happens — but that's currently only possible on the official REPL, not on Ammonite.

@Blaisorblade
Copy link
Contributor

Amazing. With -Xprint:parser, I discovered that the official REPL adds import $line4.$read.$iw.$iw.Foo; or import $line3.$read.$iw.$iw.Foo; to import one or the other Foo in later statements that use it, taking the namespace of the identifier into account.

I'm scar(r)ed O_O.

@lihaoyi
Copy link
Member

lihaoyi commented Aug 31, 2015

lol the REPL is scary isn't it.

I suspect this won't be too hard to fix - just keep track of two namespaces in the previousImports map instead of just one - but Scala's import behavior has always surprised me

@paulp
Copy link

paulp commented Nov 16, 2015

Nothing quite so terrifying as working code I guess. In my case the shadowing happens like

import pkg1._ // this has trait Order, no terms
import pkg2._ // this has object Order, no types
new Order     // not found: type Order

@paulp
Copy link

paulp commented Nov 19, 2015

It's pretty incredible that I can articulate what is the challenge and then you can swoop in a year and a half later and declare it "fixed" while falling prey to exactly the problem I described.

g-cvr-080501-mission-10a grid-6x2 1

@lihaoyi
Copy link
Member

lihaoyi commented Nov 19, 2015

I don't need to fix it forever, just enough to get my second term

@lihaoyi
Copy link
Member

lihaoyi commented Nov 19, 2015

Hopefully it won't get shadowed by someone else's second type though

@som-snytt
Copy link

On surprising behavior of imports generally, I almost commented on paulp's recent ticket, then realized I could explain one behavior but not the other, and then I had other stuff to do that day.

https://issues.scala-lang.org/browse/SI-9552

If you allow mucking with the template, with user-mucked wrapping of the expression, then automation feels even more challenging.

A while ago I had a PR with an :imports command that gave you some control, quarantining symbols for templating purposes. Maybe the other model is control over history: people want to selectively forget results (for purposes of garbage collection), which could also be handy for cleaning up namespaces.

@lihaoyi lihaoyi added the bug label Dec 1, 2015
@lihaoyi lihaoyi closed this as completed in 3645628 Feb 1, 2016
@paulp
Copy link

paulp commented Feb 3, 2016

Doesn't appear to be fixed at all. Here's a test case, and what it does in the scala repl.

package p1 {
  class Order
}

package p2 {
  object Order {
    def apply(x: p1.Order): Unit = println("ok!")
  }
}
// Welcome to Scala version 2.11.7 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_72).
// Type in expressions to have them evaluated.
// Type :help for more information.
//
// scala> import p1._, p2._ ; Order(new Order)
// ok!

Here's ammonite 0.5.3 failing.

@ import p2._, p1._
import p2._, p1._
@ Order(new Order)
Main.scala:44: not found: value Order
Order(new Order)
^
Compilation Failed
@ p2.Order
res1: p2.Order.type = p2.Order$@49a94903
@ new p1.Order
res2: Order = p1.Order@4ceb815f
@

@lihaoyi lihaoyi reopened this Feb 3, 2016
@lihaoyi
Copy link
Member

lihaoyi commented Feb 3, 2016

What I've found so far:

  • I need to filter .allImportedSymbols based on sym.exists. Why am I getting non-existent symbols? GOK
  • The logic around deciding the name of an import-selector-name is wrong. .isTypeName is never true. May need to dig into the symbols and types and figure that out myself

@lihaoyi
Copy link
Member

lihaoyi commented Feb 3, 2016

image

@paulp
Copy link

paulp commented Feb 3, 2016

Yes all imported names are created as TermNames before it is known what they actually are.

lihaoyi pushed a commit that referenced this issue Feb 3, 2016
@lihaoyi
Copy link
Member

lihaoyi commented Feb 3, 2016

@paulp want to try playing around with and/or code-reviewing ac04b84? I honestly have no clue what I'm doing with the compiler internals and don't really have a real test plan, so i just randomly permuted things that previously failed with the 0.5.3 and it seems to not fail anymore.

image

@lihaoyi
Copy link
Member

lihaoyi commented Feb 3, 2016

AFAICT it's already an improvement but if you find other things which are easily fixed in the interim then might as well fix all those before publishing 0.5.4

@paulp
Copy link

paulp commented Feb 3, 2016

I can give you some high level advice: work in terms of imported and defined symbols, not names. A reference to a name in some namespace then resolves to the most recently imported symbol which has that name.

I would also use something like ChainMap to preserve the history of each name in the repl. That branch is a work in progress from when I was toying with forking ammonite. It's intended to provide a better foundation for solving the problem in this ticket. Feel free to make use of any of it.

@lihaoyi
Copy link
Member

lihaoyi commented Feb 3, 2016

Ok thanks! Perhaps in a future diff I'll make use of that. For now I'll publish what I have first since it fixes obvious (easy to hit!) brokenness soonish and then maybe spend time ratholing in scala-compiler-details later...

@lihaoyi lihaoyi closed this as completed Feb 3, 2016
@paulp
Copy link

paulp commented Feb 3, 2016

@lihaoyi Possibly of greater value is a second commit I just pushed, 09cbeea, which includes an overhaul of the compiler plugin (amongst a speculative and probably doomed effort to achieve some novel type safety properties.)

@lihaoyi
Copy link
Member

lihaoyi commented Feb 3, 2016

@paulp Thanks a lot! I'll take a look when I get home from removing dead snakes at work today

lihaoyi pushed a commit that referenced this issue Feb 3, 2016
@som-snytt
Copy link

As an orthogonal issue, my previous comment meant to say, import in a repl doesn't need to map to the language construct. It's a way for the user to express "what is currently in scope", irrespective of how that is expressed in code. For example, import X._ could nullify a previous import X.a in an enclosing scope, etc. I formulated that once as a command :import, but maybe that's overkill, and what's wanted in a repl is a very simple model.

@som-snytt
Copy link

FSR, my office snakes are live. Like Indiana Jones, "I hate snakes," etc. I don't actually hate snakes, speaking metaphorically.

I think we need a photo of Miles's dog as a type astronaut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants