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

Implement trait associated functions #805

Merged
merged 3 commits into from
Nov 23, 2022

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Nov 4, 2022

What was wrong?

We don't have trait associated functions.

How was it fixed?

Basically just enhanced expr_call_path to keep looking for candidates that are implemented via traits if we can not resolve the path directly on the type.

This also includes #815 which can be reviewed seperately.

@cburgdorf cburgdorf force-pushed the christoph/feat/trait_ass_fn branch 4 times, most recently from 49c2972 to 7206089 Compare November 21, 2022 09:54
@cburgdorf cburgdorf changed the title Christoph/feat/trait ass fn Implement trait associated functions Nov 21, 2022
@cburgdorf cburgdorf marked this pull request as ready for review November 21, 2022 09:57
@@ -1,7 +1,7 @@
use crate::display::Displayable;
use crate::errors::{FatalError, IndexingError};
use crate::namespace::items::{FunctionId, FunctionSigId, ImplId, Item, StructId, TypeDef};
use crate::namespace::scopes::BlockScopeType;
use crate::namespace::scopes::{check_visibility, BlockScopeType};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be moved on the AnaylzerContext instead of importing it here?

Comment on lines +6 to +15
error: Applicable items exist but are not in scope
┌─ compile_errors/trait_not_in_scope2/src/foo.fe:11:6
11 │ fn do() {
│ ^^ candidate #1 is defined here on trait `DoThing`
·
16 │ fn do() {
│ ^^ candidate #2 is defined here on trait `DoOtherThing`
= Hint: Bring one of these candidates in scope via `use module_name::trait_name`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@@ -995,8 +995,139 @@ fn expr_call_path<T: std::fmt::Display>(
generic_args: &Option<Node<Vec<fe::GenericArg>>>,
args: &Node<Vec<Node<fe::CallArg>>>,
) -> Result<(ExpressionAttributes, CallType), FatalError> {
let named_thing = context.resolve_path(path, func.span)?;
expr_call_named_thing(context, named_thing, func, generic_args, args)
match context.resolve_any_path(path) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is resolve_any_path called instead of resolve_visible_path? If a "normal" associated function is not visible, it should not be counted as a callable candidate.

For example, there is no ambiguity in the following example, but the compiler rejects it.

// a.fe
pub struct MyS {
    fn x() -> u256 {
        return 10
    }
}

// main.fe
use a::MyS

trait Trait {
    fn x() -> u256;
}

impl Trait for MyS {
    fn x() -> u256 {
        return 10
    }
}

fn main() -> u256 {
    return MyS::x()
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Eagle eyes! 🦅 I pushed a solution in another commit. I'm not super happy with it but I'm not very creative this morning.

Copy link
Member

@Y-Nak Y-Nak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems great! I just left one nit.

}
};

if x.is_err() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this leftover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops...indeed.

@cburgdorf cburgdorf merged commit 92273c3 into ethereum:master Nov 23, 2022
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 this pull request may close these issues.

2 participants