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

Default trait methods do not have access to imports from their declaring file. #2150

Closed
chalcolith opened this issue Aug 10, 2017 · 10 comments · Fixed by #4027
Closed

Default trait methods do not have access to imports from their declaring file. #2150

chalcolith opened this issue Aug 10, 2017 · 10 comments · Fixed by #4027
Assignees
Labels
bug Something isn't working

Comments

@chalcolith
Copy link
Member

If I have the following files:

// foo.pony

use "debug"

trait Foo[T: Stringable #read]
  fun foo(): this->T

  fun bar() =>
    Debug.out(foo().string())
// bar.pony

class Bar[T: Stringable #read] is Foo[T]
  let _t: T

  new create(t: T) =>
    _t = t

  fun foo(): this->T =>
    _t
// main.pony

actor Main
  new create(env: Env) =>
    let b = Bar[I32](123)
    b.bar()

I get the following error message:

Error:
C:\Users\Gordon\Dev\pony\privdebug\privdebug\foo.pony:8:5: can't find declaration of 'Debug'
    Debug.out(foo().string())
    ^

If I add use "debug" to the top of bar.pony, the error goes away. I assume that traits' default methods' AST gets copied to where they are instantiated, but then they lose access to the imports from their original file.

@SeanTAllen
Copy link
Member

Yeah, we've experienced this at Sendence. You need the import in the file where it gets used which... isn't right but it's a temporary work around. You nicely found a minimal case for us.

@jemc
Copy link
Member

jemc commented Aug 10, 2017

So, this is an interesting one.

We need the type resolution that happens in the names_nominal function to happen prior to the traits pass (where default method bodies are copied into the other types). It happens in the names pass for TK_NOMINAL types in type expressions, which is fine and dandy for those, but nominal types in value expressions are still TK_REFERENCE until they hit the refer pass, and don't get names_nominal until the expr pass. The refer and expr passes happen after the traits pass, so it's a no-go for those.

Not sure off the top of my head what the best approach for a fix is, but that's the problem in brief.

@mfelsche
Copy link
Contributor

I did hit the same one.

Just out of curiosity, would this also fail if the use statement would be written like: use dbg = "debug" and references to the debug package would be explicitly written as dbg.Debug.out(...)?

@SeanTAllen
Copy link
Member

@mfelsche yes, you would have the same problem.

@ergl
Copy link
Member

ergl commented Jun 9, 2021

As raised today on Zulip, this bug also happens with interfaces.

@jemc
Copy link
Member

jemc commented Jun 29, 2021

I think the best way to fix this would be to split off the part of the refer pass that deals with type names into a new pass called refer_type which does that part, prior to the traits pass, while keeping the rest of the refer pass happening in its current position after the traits pass.

This is how I solved a similar issue in the Mare compiler.

@SeanTAllen
Copy link
Member

@jemc

split off the part of the refer pass that deals with type names into a new pass called refer_type which does that part

I looked at refer.c and nothing jump out as "the part of the refer pass that deals with type names". What did you mean?

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 11, 2022
@SeanTAllen
Copy link
Member

SeanTAllen commented Feb 11, 2022

refer_reference to names pass?

ast_result_t pass_names(ast_t** astp, pass_opt_t* options)

also handle TK_REFERENCE.

If lowercase name, don't touch.
If uppercase name (aka type), then do refer_reference changes here (but not checks).
could be private so _Upper
('check_id_type`)

case TK_INTERFACE:
case TK_TRAIT:
case TK_TYPE:
case TK_TYPEPARAM:
case TK_PRIMITIVE:
case TK_STRUCT:
case TK_CLASS:
case TK_ACTOR:
{
  ast_setid(ast, TK_TYPEREF);

  ast_add(ast, ast_from(ast, TK_NONE));    // 1st child: package reference
  ast_append(ast, ast_from(ast, TK_NONE)); // 3rd child: type args

  return true;
}

come into names.

@SeanTAllen SeanTAllen self-assigned this Feb 13, 2022
@SeanTAllen SeanTAllen removed the help wanted Extra attention is needed label Feb 15, 2022
@SeanTAllen
Copy link
Member

I'm working on this with some assistance from @jemc.

@SeanTAllen
Copy link
Member

SeanTAllen commented Feb 21, 2022

I have a fix for this. I need to clean it up as its two bunches of identical code copied into two locations, I'll have a PR open sometime this week. FYI the notes above for how to address aren't the path I ended up using as I ran into issues with the original idea.

SeanTAllen added a commit that referenced this issue Feb 22, 2022
…m default method bodies (#4027)

We've had issues with symbols within trait and interface default method bodies. If symbols used within those bodies were not available in the local scope that the method bodies were added to, the symbols couldn't be found. 

The issue was that we were only searching the local scope. 

This change adds an additional AST symbol lookup strategy where we look for a function or behavior as our parent. If one exists, we check to see if it was provided by a trait. This is done by looking to see if the AST data is present. If it is, then that is the AST for original trait implementation of the method. We can then use that AST node to find symbols that were present at the original declaration site of the trait.

Fixes #3737 
Fixes #2150
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
6 participants