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

Comments

Projects
None yet
5 participants
@mpilquist

mpilquist commented Aug 26, 2015

› ./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

This comment has been minimized.

Contributor

Blaisorblade commented Aug 31, 2015

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

This comment has been minimized.

Contributor

Blaisorblade commented Aug 31, 2015

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

This comment has been minimized.

Contributor

Blaisorblade commented Aug 31, 2015

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

This comment has been minimized.

Owner

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Owner

lihaoyi commented Nov 19, 2015

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

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Nov 19, 2015

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

@som-snytt

This comment has been minimized.

som-snytt commented Nov 19, 2015

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 in 3645628 Feb 1, 2016

@paulp

This comment has been minimized.

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

This comment has been minimized.

Owner

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

This comment has been minimized.

Owner

lihaoyi commented Feb 3, 2016

image

@paulp

This comment has been minimized.

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

This comment has been minimized.

Owner

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

This comment has been minimized.

Owner

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

This comment has been minimized.

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

This comment has been minimized.

Owner

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 Feb 3, 2016

@paulp

This comment has been minimized.

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

This comment has been minimized.

Owner

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

This comment has been minimized.

som-snytt commented Feb 3, 2016

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

This comment has been minimized.

som-snytt commented Feb 3, 2016

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.

@paulp paulp referenced this issue Mar 4, 2017

Closed

Import shadowing #65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment