ship precise rust to dotcom #586
Conversation
|
@aymannadeem I think the main thing is adding |
patrickt
left a comment
patrickt
left a comment
There was a problem hiding this comment.
This is a great start!
My recommendation, before you start fixing anything mentioned below, is to come up with a set of example .rs files that collectively exercise all the syntax types for which you’ve defined instances, commit them to a folder in the repository, and (ideally, but perhaps optionally) define .json fixtures and check that they’re commensurate with the tags output. This has the advantage of giving you a tight compile-test cycle as well as a guide to what we need and don’t need to tag.
| _ -> pure () | ||
| case name of | ||
| EPrj (Rust.Identifier {text, ann}) -> yield text ann | ||
| EPrj (Rust.Metavariable {text, ann}) -> yield text ann |
There was a problem hiding this comment.
Do we know exactly what a Metavariable appearing as the name of a function entails? Is it always going to be useful information?
| case returnType of | ||
| Just (Parse.Success type') -> gtags type' | ||
| _ -> pure () | ||
| case typeParameters of | ||
| Just (Parse.Success typeParams) -> gtags typeParams | ||
| _ -> pure () |
There was a problem hiding this comment.
It occurs to me that you can elide these pattern matches entirely and just rely on gtags—instead of calling gtags block at the end, call gtags t, and the returnType and typeParameters and all its friends will be iterated automatically. (But check before and after and make sure the resulting trees are still correct!)
| ann = loc@Loc {byteRange}, | ||
| body = Parse.Success bod, | ||
| name = Parse.Success (Rust.TypeIdentifier {text, ann}) | ||
| } = yieldTag text P.FUNCTION P.DEFINITION ann byteRange >> gtags t |
There was a problem hiding this comment.
Definitely not P.FUNCTION here, since this is a case of an enumeration… but honestly, I have no idea which SyntaxType we should use for this case. Maybe we should add a new one? Perhaps @tclem has some thoughts.
| { ann = loc@Loc {byteRange}, | ||
| name = Parse.Success (Rust.FieldIdentifier {text, ann}), | ||
| type' = Parse.Success type'' | ||
| } = yieldTag text P.FUNCTION P.DEFINITION ann byteRange >> gtags t |
| EPrj (Rust.Identifier {text, ann}) -> yield text ann | ||
| EPrj (Rust.Metavariable {text, ann}) -> yield text ann | ||
| where | ||
| yield name ann = yieldTag name P.FUNCTION P.DEFINITION ann byteRange >> gtags t |
There was a problem hiding this comment.
Hard to say whether this is the right thing without knowing what a FunctionSignatureItem constitutes.
| _ -> pure () | ||
| gtags t | ||
| where | ||
| yield function ann = yieldTag function P.FUNCTION P.DEFINITION ann byteRange >> gtags t |
There was a problem hiding this comment.
Since you only use this yield call in one place, you could remove it from the where if you liked.
| instance ToTags Rust.StructItem where | ||
| tags | ||
| t@Rust.StructItem | ||
| { | ||
| ann = loc@Loc {byteRange}, | ||
| body, | ||
| name = Parse.Success (Rust.TypeIdentifier {text, ann}) | ||
| } = yieldTag text P.FUNCTION P.REFERENCE ann byteRange >> gtags t | ||
|
|
||
| instance ToTags Rust.UnionItem where | ||
| tags | ||
| t@Rust.UnionItem | ||
| { | ||
| ann = loc@Loc {byteRange}, | ||
| body, | ||
| name = Parse.Success (Rust.TypeIdentifier {text, ann}) | ||
| } = yieldTag text P.FUNCTION P.DEFINITION ann byteRange >> gtags t | ||
|
|
||
| instance ToTags Rust.TraitItem where | ||
| tags | ||
| t@Rust.TraitItem | ||
| { | ||
| ann = loc@Loc {byteRange}, | ||
| body, | ||
| name = Parse.Success (Rust.TypeIdentifier {text, ann}) | ||
| } = yieldTag text P.FUNCTION P.DEFINITION ann byteRange >> gtags t |
There was a problem hiding this comment.
Similar uses of P.FUNCTION here that we don’t want.
| tags | ||
| t@Rust.CallExpression | ||
| { | ||
| ann = loc@Loc {byteRange}, | ||
| function = Parse.Success expr, | ||
| arguments = Parse.Success args | ||
| } = gtags expr | ||
| tags _ = pure () |
There was a problem hiding this comment.
Here, too, I think we can just call gtags t—or, since that’s equivalent to the default instance, drop it entirely.
|
Going to pause this work for now in leiu of https://github.com/github/aleph/pull/503 |
Data.LanguageParsing.Parserhaskell.ymlToTagsinstances forLanguage.Rust.Tags@github/semantic-code can someone with more context/experience on this point me to things I've missed?