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

Fix empty code blocks when trait used interface method #275

Merged
merged 4 commits into from
Sep 29, 2021

Conversation

sezna
Copy link
Contributor

@sezna sezna commented Sep 27, 2021

Closes #241

Consider a trait declaration:

trait MyTrait {
  some_interface_fn() -> bool;
} { 
  some_method() -> bool { 
    some_interface_fn()
  }
}

In order for some_method to type check its call to some_interface_fn, we need to insert some_interface_fn into the namespace before we actually have a function body for it. I used to_dummy_func(), which would take a typed trait function and insert it with an empty body. In the original design, this didn't matter because it was thrown away before code generation.

Since then, we have added a copy of function declarations inside of function applications for ease of code generation. This was capturing the empty code block and keeping it around until code generation, causing weird bugs. This change throws away any dummy functions and makes trait declarations store FunctionDeclarations instead of TypedFunctionDeclarations, forcing a (correct) type-check when a trait is implemented and those function bodies actually exist.

This change also removes where clauses for now, since their implementation relied on a faulty usage of to_dummy_func. Those will have to be implemented as a part of #272

@sezna sezna added bug Something isn't working compiler General compiler. Should eventually become more specific as the issue is triaged labels Sep 27, 2021
@sezna sezna self-assigned this Sep 27, 2021
@sezna sezna marked this pull request as draft September 28, 2021 02:00
@otrho otrho marked this pull request as ready for review September 28, 2021 02:21
@otrho otrho marked this pull request as draft September 28, 2021 02:24
@otrho
Copy link
Contributor

otrho commented Sep 28, 2021

I'm now struggling to get a mental model of how all this should work. I'm going to talk out loud here to see if I can make sense of it.

We have a DAG of Namespace, one for each module, so there's std -> ops and in there is e.g. the declaration for the Ord trait and also the implementations of Ord for the standard types.

And there's the namespace for the current module. We use callpaths like std::ops::Ord to find symbols in other namespaces or use statements as shortcuts.

Operations like a == b will translate to a.eq(b) where eq() is declared in the std::ops::Ord trait.

A local implementation of a trait will go into the current namespace, into a special collection of trait implementations. enum A { ... } is local, so is impl Ord for A { ... }. [Ahem, not quite true, see my next comment below.] In this case Ord is found by using either impl std::ops::Ord ... or use std::ops::Ord; impl Ord ....

If we try to use A::eq(), i.e., the trait implementation of Ord for A we need to look up its type. We should find this in the current namespace in the collection of implemented traits. This collection is slightly different to a normal symbol lookup because it associates the type being implemented for, e.g., A along with the symbol, to find the function decl type.

Now that I've typed this out it makes sense to me. While I was looking at the code it didn't. @sezna is what I've outlined above correct?

@otrho
Copy link
Contributor

otrho commented Sep 28, 2021

OK, what I think is actually happening, as of the tip of this branch, is we have insert_trait_implementation() which takes a fully qualified trait name, e.g., std::ops::Ord, it finds the namespace for that trait and inserts into it the passed trait implementation, i.e., the type the trait is implemented for and the function declarations.

This isn't what I described above, where I suggested a local implementation goes into the local namespace.

I experimented with a change, which instead did insert the implementation locally, and looked it up locally and it fixed the problem with the if_elseif_enum test... but it broke the std::ops::Ord implementations for standard types, because the standard types aren't in the local namespace, nor are they fully qualified to be found in std::ops.

I think this is the core of the issue, and is essentially what I was trying to fix in #266. Typically we want implementations of traits to exist where they're declared, rather than pollute the namespace where the trait is declared. (I think?) But the standard types essentially are local (to everywhere) and so looking up their methods is special. This is kinda the crux of #187 in the first place.

Damn, reading #187 again is essentially describing the problem we still have.

How to have local trait implementations for local types but still to find the methods for standard types? Perhaps the solution is actually to put all the standard types into a namespace?

We now insert trait implementations into the namespace where they're
declared, rather than where the trait is declared.  This involves using
a full callpath to the trait as the key for the trait lookup hashmap.

We therefore also lookup trait implementations in the local namespace to
resolve them.

Unfortunately this breaks trait implementations for standard types, like
those found in `std::ops` for `std::ops::Ord` because without a fully
qualified path to the types we will look for their trait implementations
locally and not find them.

A hack has been added to the lookup function which will first try the
local namespace and then if unsuccessful try the module where the trait
is declared.  This works for all our current tests, but is fragile and
it's bad to assume that we'll always find implementations for standard
types where the trait is declared.
@otrho
Copy link
Contributor

otrho commented Sep 28, 2021

I've pushed a commit with the change mentioned above, which breaks for std::ops::Ord for standard types... except I added a little hack to try harder to resolve those methods.

It passes all the tests now, but if there's a better way to do this then we should work it out.

@sezna
Copy link
Contributor Author

sezna commented Sep 28, 2021

I'm going to say this is good for now (if we have more pains later, we can revisit this, not a high priority), because I can't think of an example outside of implementations for primitive types where the types would not be in scope for either the trait or the place it is implemented.

@sezna sezna marked this pull request as ready for review September 28, 2021 14:46
@sezna sezna merged commit 0663ead into master Sep 29, 2021
@sezna sezna deleted the sezna/241-take-2 branch September 29, 2021 04:47
emilyaherbert pushed a commit that referenced this pull request Sep 29, 2021
* Fix empty code blocks when trait used interface method

* remove unused imports

* Change the way trait implementations are handled by the namespace.

We now insert trait implementations into the namespace where they're
declared, rather than where the trait is declared.  This involves using
a full callpath to the trait as the key for the trait lookup hashmap.

We therefore also lookup trait implementations in the local namespace to
resolve them.

Unfortunately this breaks trait implementations for standard types, like
those found in `std::ops` for `std::ops::Ord` because without a fully
qualified path to the types we will look for their trait implementations
locally and not find them.

A hack has been added to the lookup function which will first try the
local namespace and then if unsuccessful try the module where the trait
is declared.  This works for all our current tests, but is fragile and
it's bad to assume that we'll always find implementations for standard
types where the trait is declared.

Co-authored-by: Alexander Hansen <alexanderhansen@Alexanders-MacBook-Pro.local>
Co-authored-by: Toby Hutton <toby@grusly.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default implementations for Ord (and others?) fail.
2 participants