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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement trait
#39
Implement trait
#39
Conversation
My first instinct would be to avoid adding |
Good point. I would expect the typical usage is to read those structs (from a parsed result), but there may be use cases where people create them (for testing, or local manipulation). I removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -1289,3 +1289,41 @@ fn parse_mod() { | |||
let mod_decl = parse_declaration_checked(expr); | |||
assert_debug_snapshot!(mod_decl); | |||
} | |||
|
|||
// ================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add a test for something like:
trait Foo {
type X = i32;
const N: i32 = 42;
}
Eg a trait declaration which is semantically invalid but syntactically valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we already test those, with SHORT_NAME
and TypeWithDefault
?
const LONG_NAME: &'static str;
const SHORT_NAME: char = 'T';
type AssocType: Bound;
type TypeWithDefault = Rc<RefCell<MyStruct>>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be more expressive to add a test where those are in a trait body, to show the range of accepted syntax?
Eh, it doesn't really matter. The types are orthogonal enough that you can probably assume that if some construct parses in one context, it parses in another.
Parses traits, largely reusing the
impl
implementation. I added code toparse_impl.rs
, didn't have a new good name for the file, so I left it for now...Unfortunately this introduces a breaking change, because existing
TyDefinition
did not account for: Bounds
and optional= initializer
. In general, we should probably mark everything#[non_exhaustive]
to at least have the option to add fields in non-breaking ways.But no urgency from my side for any release, just thought I'd implement this while I'm in the flow 馃槈