enso-org / enso

Hybrid visual and textual functional programming.
https://enso.org
Apache License 2.0
7.31k stars 318 forks source link

Imported name shadowed by local type not reported #8868

Open radeusgd opened 5 months ago

radeusgd commented 5 months ago

Within a project with 2 files:

Mod.enso

type A
   Ctor1

make_a = A.Ctor1

and Main.enso

from Standard.Base import all

import project.Mod.A 
from project.Mod import make_a

type A
    Ctor2

foo (a : A) -> A = a

main =
    IO.println (foo (A.Ctor2))
    IO.println (foo make_a)

running Main.enso will result in:

Ctor2
Execution finished with an error: Type error: expected `a` to be A, but got A.
        at <enso> Main.foo(src\Main.enso:9:1-20)
        at <enso> Main.main<arg-1>(src\Main.enso:13:17-26)
        at <enso> Main.main(src\Main.enso:13:5-27)

There's 2 issues here:

  1. The imported name A is shadowed by the type definition type A but this is not reported. If not error, at least a warning should be reported to the user about the shadowing.
  2. The Type_Error message is confusing: expected A but got A - ???. The problem is that the qualified name of the types differs. We should record the qualified name of each type and if the short-names are equal, we should display the qualified names to allow the user to disambiguate types with same short names.
    ### Tasks
    - [ ] Shadowing should display warning
    - [ ] Improve type error message
radeusgd commented 5 months ago

Moreover, adding make_a = A.Ctor2 to Main.enso makes the program run:

Ctor2
Ctor2

because it also shadows the imported make_a.

This shadowing also does not issue any error nor warning.

I think shadowed symbols (methods and types) should result in a warning.

Akirathan commented 2 months ago

According to @4e6 and @JaroslavTulach, type shadowing is perfectly fine. Imagine you would like to define your Boolean type. In that case, you would not be able to import all symbols from Standard.Base. Nevertheless, it should at least report a warning, as shadowing might not always be the desired behavior.

The type error message is indeed confusing. I will try to improve it.

enso-bot[bot] commented 2 months ago

Pavel Marek reports a new STANDUP for today (2024-05-03):

Progress: - Writing tests and test utilities to create a whole temporary project structure. It should be finished by 2024-05-07.

radeusgd commented 2 months ago

According to @4e6 and @JaroslavTulach, type shadowing is perfectly fine. Imagine you would like to define your Boolean type. In that case, you would not be able to import all symbols from Standard.Base.

That's exactly the rationale behind the hiding construct.

If you want to define your own Boolean - that's perfectly fine, you need to use:

from Standard.Base import all hiding Boolean
Akirathan commented 3 weeks ago

Marking this issue as x-on-hold, as it would be very useful to have more specific information in BindingsMap.resolvedImports. For example, for these two modules:

local.Proj.Mod:

type T

local.Proj.Main:

from local.Proj.Mod import T

if we look into the bindings map of local.Proj.Main, there is a ResolvedImport with the target of ResolvedModule("local.Proj.Mod"), but it should rather be ResolvedType("local.Proj.Mod.T").

Adding this kind of information to the BindingsMap is tracked in https://github.com/enso-org/enso/issues/6729

Without this additional information, implementing a static linting compiler pass to detect shadowing of imported symbols is not worth the effort at the moment.

Closing the associated PR #9863 with tests and a skeleton of the linting pass. Let's re-open it once we have a better BindingMap.