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

in inside a template not lowered to contains if called from another module #18150

Closed
timotheecour opened this issue Jun 2, 2021 · 5 comments · Fixed by #24315
Closed

in inside a template not lowered to contains if called from another module #18150

timotheecour opened this issue Jun 2, 2021 · 5 comments · Fixed by #24315

Comments

@timotheecour
Copy link
Member

timotheecour commented Jun 2, 2021

Example

# t12344b.nim:
import std/sets
template foo*[T](a: T) =
# proc foo*[T](a: T) = # works
  var s: HashSet[T]
  # echo contains(s, a) # works
  echo a in s # BUG
when isMainModule: foo(1) # works

# t12344.nim:
import t12344b
foo(1)

nim r t12344b # works
nim r t12344 # bug

Current Output

Error: type mismatch: got <HashSet[system.int], int literal(1)>

Expected Output

works

Possible Solution

lower in to contains in semTemplateDef (in generic pre-pass, it works, so semgnrc does the right thing here)

Additional Information

@Araq
Copy link
Member

Araq commented Jun 2, 2021

There is no bug here, template expansion is well defined.

@timotheecour
Copy link
Member Author

timotheecour commented Jun 2, 2021

but what is a in b if not operator sugar for contains(b, a) [1]?

this makes in inside public templates not usable unless you expose your internal details and require client code to also import std/sets (etc) whenever you use foo

The situation is similar to for a in b which doesn't work in generics in similar conditions (refs #11167 ), forcing you to use for a in items(b).

How is the suggested lowering worse than the current situation?

[1] refs: https://nim-lang.github.io/Nim/system.html#contains%2Cset%5BT%5D%2CT

One should overload this proc if one wants to overload the in operator.
The parameters are in reverse order! a in b is a template for contains(b, a). This is because the unification algorithm that Nim uses for overload resolution works from left to right. But for the in operator that would be the wrong direction for this piece of code: [...]

@Araq
Copy link
Member

Araq commented Jun 3, 2021

How is the suggested lowering worse than the current situation?

A simple language with easy to explain quirks as the result of these simple rules is much better than a language that has many special cases that ends up with just the same amount of quirks. For example, every smart rewrite rule that we do has a direct impact on the understandability of Nim's macro system.

However, if Nim did template expansions eagerly, this would solve the problem too, without special casing for the in shortcut.

The situation is similar to for a in b which doesn't work in generics in similar conditions (refs #11167 ), forcing you to use for a in items(b).

The solution for this gotcha is to deprecate the implicit "items" altogether, not to introduce even more special cases.

@n0bra1n3r
Copy link

n0bra1n3r commented Jun 3, 2021

I wanted to add my 2 cents and mention that a similar thing happens with dotOperators in templates and generic procs. I end up having to do something like cppObject.`.()`(CppMethod) inside a template/generic declared in one module that is called from another module that doesn't import the dotOperator implementations. If this is not intended (or a totally separate issue), I can file a separate bug.

I wanted to mention that I encountered something like this in D, but that language has 'local imports', so I could import things from inside templates. Can't imagine how difficult that would be to implement in Nim given how complex the macro system is, so maybe something like bind could work for cases like this?

@Araq
Copy link
Member

Araq commented Jun 4, 2021

I wanted to mention that I encountered something like this in D, but that language has 'local imports', so I could import things from inside templates. Can't imagine how difficult that would be to implement in Nim given how complex the macro system is, so maybe something like bind could work for cases like this?

Local imports were so problematic in D that they had to change the scoping rules just for them, I don't want to repeat their mistakes. The real solution is a bind statement, as you said.

dsrw added a commit to dsrw/print that referenced this issue Dec 1, 2021
@Araq Araq closed this as completed in 2864830 Oct 25, 2024
narimiran pushed a commit that referenced this issue Jan 14, 2025
closes nim-lang/RFCs#380, fixes #4773, fixes
#14729, fixes #16755, fixes #18150, fixes #22984, refs #11167 (only some
comments fixed), refs #12620 (needs tiny workaround)

The compiler gains a concept of root "nominal" types (i.e. objects,
enums, distincts, direct `Foo = ref object`s, generic versions of all of
these). Exported top-level routines in the same module as the nominal
types that their parameter types derive from (i.e. with
`var`/`sink`/`typedesc`/generic constraints) are considered attached to
the respective type, as the RFC states. This happens for every argument
regardless of placement.

When a call is overloaded and overload matching starts, for all
arguments in the call that already have a type, we add any operation
with the same name in the scope of the root nominal type of each
argument (if it exists) to the overload match. This also happens as
arguments gradually get typed after every overload match. This restricts
the considered overloads to ones attached to the given arguments, as
well as preventing `untyped` arguments from being forcefully typed due
to unrelated overloads. There are some caveats:

* If no overloads with a name are in scope, type bound ops are not
triggered, i.e. if `foo` is not declared, `foo(x)` will not consider a
type bound op for `x`.
* If overloads in scope do not have enough parameters up to the argument
which needs its type bound op considered, then type bound ops are also
not added. For example, if only `foo()` is in scope, `foo(x)` will not
consider a type bound op for `x`.

In the cases of "generic interfaces" like `hash`, `$`, `items` etc. this
is not really a problem since any code using it will have at least one
typed overload imported. For arbitrary versions of these though, as in
the test case for #12620, a workaround is to declare a temporary
"template" overload that never matches:

```nim
# neither have to be exported, just needed for any use of `foo`:
type Placeholder = object
proc foo(_: Placeholder) = discard
```

I don't know what a "proper" version of this could be, maybe something
to do with the new concepts.

Possible directions:

A limitation with the proposal is that parameters like `a: ref Foo` are
not attached to any type, even if `Foo` is nominal. Fixing this for just
`ptr`/`ref` would be a special case, parameters like `seq[Foo]` would
still not be attached to `Foo`. We could also skip any *structural* type
but this could produce more than one nominal type, i.e. `(Foo, Bar)`
(not that this is hard to implement, it just might be unexpected).

Converters do not use type bound ops, they still need to be in scope to
implicitly convert. But maybe they could also participate in the nominal
type consideration: if `Generic[T] = distinct T` has a converter to `T`,
both `Generic` and `T` can be considered as nominal roots.

The other restriction in the proposal, being in the same scope as the
nominal type, could maybe be worked around by explicitly attaching to
the type, i.e.: `proc foo(x: T) {.attach: T.}`, similar to class
extensions in newer OOP languages. The given type `T` needs to be
obtainable from the type of the given argument `x` however, i.e.
something like `proc foo(x: ref T) {.attach: T.}` doesn't work to fix
the `ref` issue since the compiler never obtains `T` from a given `ref
T` argument. Edit: Since the module is queried now, this is likely not
possible.

---------

Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit 2864830)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants