Closed dennwc closed 6 years ago
Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.
driver/normalizer/annotation.go, line 20 at r1 (raw file):
The `Incomplete` role must not be applied automatically. It's used by the driver programmer to mark roles that doesn't express the full semantic information for any reason (usually because of lack of more specific roles). PTAL: https://github.com/bblfsh/documentation/blob/master/proposals/bip-002.md
This is exactly what it's used here for. FieldList
appears either as a list of arguments for a function (and will not receive this role, since it already has it), or a list of struct fields, that is already annotated in other ways, I believe. It might make sense to mark them as List, Incomplete
, but overall I think it works as it should.
driver/normalizer/annotation.go, line 21 at r1 (raw file):
The Unannotated role on the other side should be applied automatically. Having do add a Transformer in the driver breaks a little BIP-2 since it should be automatically done despite whatever the driver programmer does, but I guess we can work with this (opening an issue if the current SDK get's merged) until we find a way to apply it automatically. (same for the other drivers).
I agree that this should be added automatically, but for now I'm waiting for you to verify that it works well for Python and PHP. The reason why it might not work, is because it adds this role to every node without roles, and may touch some nodes that was not meant to be annotated at all (legacy IsNode
method was filtering it before). In that case the call to Unannotated
might accept a function that will tell what to annotate and what to ignore.
On the other hand I think better sollution would be to return Unannotated
role for Node.Roles()
when the field does not exists. It will allow to remove this additional transformation (and improve prerformance by not scanning the tree again), and will automatically work for all nodes.
Comments from Reviewable
Review status: 0 of 141 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.
driver/normalizer/annotation.go, line 20 at r1 (raw file):
This is exactly what it's used here for. `FieldList` appears either as a list of arguments for a function (and will not receive this role, since it already has it), or a list of struct fields, that is already annotated in other ways, I believe. It might make sense to mark them as `List, Incomplete`, but overall I think it works as it should.
The List role is for arrays, list, etc, data structures, not for list of items in the UAST or arguments.
driver/normalizer/annotation.go, line 21 at r1 (raw file):
I agree that this should be added automatically, but for now I'm waiting for you to verify that it works well for Python and PHP. The reason why it might not work, is because it adds this role to *every* node without roles, and may touch some nodes that was not meant to be annotated at all (legacy `IsNode` method was filtering it before). In that case the call to `Unannotated` might accept a function that will tell what to annotate and what to ignore. On the other hand I think better sollution would be to return `Unannotated` role for `Node.Roles()` when the field does not exists. It will allow to remove this additional transformation (and improve prerformance by not scanning the tree again), and will automatically work for all nodes.
Seem to work pretty well! I found a crashing bug while fixing a node that was unannotated because it didn't match the structure of some of it's use in the native AST but I don't think is related (I'll tell you tomorrow since I don't want to interfere much with your OSD).
Comments from Reviewable
Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.
driver/normalizer/annotation.go, line 20 at r1 (raw file):
The List role is for arrays, list, etc, data structures, not for list of items in the UAST or arguments.
This is why I omitted it, yes. Do you consider this resolved?
driver/normalizer/annotation.go, line 21 at r1 (raw file):
Seem to work pretty well! I found a crashing bug while fixing a node that was unannotated because it didn't match the structure of some of it's use in the native AST but I don't think is related (I'll tell you tomorrow since I don't want to interfere much with your OSD).
OK. And what do you think about moving this logic to uast.Node.Roles()
?
Comments from Reviewable
Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.
driver/normalizer/annotation.go, line 21 at r1 (raw file):
OK. And what do you think about moving this logic to `uast.Node.Roles()`?
It's probably the perfect place AFAIK.
Comments from Reviewable
Review status: 0 of 141 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.
driver/normalizer/annotation.go, line 20 at r1 (raw file):
This is why I omitted it, yes. Do you consider this resolved?
No, since it still auto applies the incomplete role in this specific case (FieldList with no roles) :) (or maybe I'm not understanding what it does.)
Comments from Reviewable
Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.
driver/normalizer/annotation.go, line 20 at r1 (raw file):
No, since it still auto applies the incomplete role in this specific case (FieldList with no roles) :) (or maybe I'm not understanding what it does.)
Again, as I said, this is intentional - FieldList
is just a proxy node. When it's used in arguments, it receives a specific role, and if not, it will get Incomplete
because we actually don't care about this particular node type.
driver/normalizer/annotation.go, line 21 at r1 (raw file):
It's probably the perfect place AFAIK.
OK, will change it in SDK
Comments from Reviewable
Review status: 0 of 141 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.
driver/normalizer/annotation.go, line 21 at r1 (raw file):
OK, will change it in SDK
Done.
Comments from Reviewable
Related to https://github.com/bblfsh/sdk/issues/243.
Signed-off-by: Denys Smirnov denys@sourced.tech