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

File local top level include #11979

Open
tkshnwesper opened this issue Apr 7, 2022 · 13 comments
Open

File local top level include #11979

tkshnwesper opened this issue Apr 7, 2022 · 13 comments

Comments

@tkshnwesper
Copy link

Bug Report

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

Context

  • There are 3 modules, A, A::B and A::C.
  • b.cr - A::B has a method def foo(x : Int32)
  • c.cr - A::C has a method def foo(x : String)
  • The spec file for A::B (b_spec.cr) only imports b.cr
  • The spec file for A::C (c_spec.cr) only imports c.cr
  • b_spec.cr - includes A::B
  • c_spec.cr - includes A::C

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
@tkshnwesper tkshnwesper added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Apr 7, 2022
@asterite
Copy link
Member

asterite commented Apr 7, 2022

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

@tkshnwesper
Copy link
Author

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

Using crystal spec

@asterite
Copy link
Member

asterite commented Apr 7, 2022

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
Copy link
Author

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

@HertzDevil
Copy link
Contributor

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
Copy link
Author

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
Copy link
Member

straight-shoota commented Apr 7, 2022

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
Copy link
Author

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
Copy link
Member

straight-shoota commented Apr 7, 2022

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
Copy link
Member

asterite commented Apr 7, 2022

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

@tkshnwesper
Copy link
Author

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

Could it be this: #4442

@daliborfilus
Copy link
Contributor

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

@tkshnwesper
Copy link
Author

tkshnwesper commented Apr 11, 2022

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

@jhass jhass changed the title Module includes happening in unexpected manner File local top level include Apr 24, 2022
@jhass jhass added kind:feature and removed kind:bug A bug in the code. Does not apply to documentation, specs, etc. labels Apr 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants