crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.42k stars 1.62k forks source link

File local top level include #11979

Open tkshnwesper opened 2 years ago

tkshnwesper commented 2 years ago

Bug Report

Repo to reproduce bug: https://github.com/tkshnwesper/potential_crystal_bug Run crystal spec

Context

When you run the test, you will get an error like

In spec/c_spec.cr:8:11

 8 | Z.new.foo("Hello")
           ^--
Error: no overload matches 'A::B::Z#foo' with type String

Overloads are:
 - A::B::Z#foo(x : Int32)

I'm not sure if it's a bug, or I am missing something. If we import only c.cr then it should use the methods defined in A::C, no? Instead it looks at A::B and pretends A::C does not exist.

Crystal version:

Crystal 1.3.2 (2022-01-18)

LLVM: 13.0.0
Default target: x86_64-apple-macosx
asterite commented 2 years ago

What does it mean "when you run the test"? Can you show us the command?

tkshnwesper commented 2 years ago

What does it mean "when you run the test"? Can you show us the command?

Using crystal spec

asterite commented 2 years ago

This will probably be the answer: when you include something at the top level, it's included on every file, not just that file.

tkshnwesper commented 2 years ago

Even though I include only A::C, does A::B get automatically included too because they are both part of A?

HertzDevil commented 2 years ago

The spec runner creates a fictitious source file that requires every spec file, so the two includes effectively have the same scope, and the last one has precedence.

tkshnwesper commented 2 years ago

The spec runner creates a fictitious source file that requires every spec file, so the two includes effectively have the same scope, and the last one has precedence.

That explains the phenomenon well, thank you. I'll check the source code to see if there's an easy fix

straight-shoota commented 2 years ago

I suppose it would be nice to have local includes per file which behave like private types in the top-level scope. Maybe that should've been the behaviour for include on the top-level in the first place. It seems to be what users would expect (I would, and the OP seems to assume that as well).

tkshnwesper commented 2 years ago

I think I get what you mean @straight-shoota , basically

include A::B
Z.new.foo(1)

include A::C
Z.new.foo("Hi")

should work in a single file, right?

straight-shoota commented 2 years ago

No. I meant it should work as in your original example. When you include A::B from b.cr, it won't be included in other files such as c.cr and vice versa.

asterite commented 2 years ago

I think I suggested private include in the past, there might be an issue for it.

tkshnwesper commented 2 years ago

I think I suggested private include in the past, there might be an issue for it.

Could it be this: https://github.com/crystal-lang/crystal/issues/4442

daliborfilus commented 2 years ago

Wouldn't this also allow significan compiler performance boost (easier change detection)?

tkshnwesper commented 2 years ago

I wrote a small macro to localize includes: https://github.com/tkshnwesper/namespace

So now, I can run

require "namespace"

namespace A::B do
  describe Z do
    it "calls foo" do
      Z.new.foo(1)
    end
  end
end

namespace A::C do
  describe Z do
    it "calls foo" do
      Z.new.foo("Hello")
    end
  end
end