Add SourceCodeInfo to lowering of IR to FDS
#656
Conversation
experimental/ir/fdp.go
Outdated
| dg.path = []int32{internal.FilePackageTag} | ||
| dg.addSourceLocation( | ||
| file.AST().Package().Span(), | ||
| file.AST().Package().KeywordToken().ID(), | ||
| file.AST().Package().Semicolon().ID(), | ||
| ) | ||
| dg.path = nil |
There was a problem hiding this comment.
I don't like this :S
Is there a better way to do this that maybe adds a helper for setting/building the path...?
There was a problem hiding this comment.
Yeah, I think that is pretty reasonable -- I kinda just built it along in-line as I went, but it is pretty rough to maintain. Related to the comment below, I'll abstract this into something a little nicer.
experimental/ir/fdp.go
Outdated
| for extn := range seq.Values(extend.Extensions()) { | ||
| fd := new(descriptorpb.FieldDescriptorProto) | ||
| fdp.Extension = append(fdp.Extension, fd) | ||
| dg.path = append(dg.path, internal.FileExtensionsTag, extnIndex) |
There was a problem hiding this comment.
This seems.. wrong? Only the first iteration will get whatever dg.path is when this loop is entered.
Reallllly don't like this api
There was a problem hiding this comment.
At the top level of the file, dg.path will always be nil to reset -- but yeah, I don't disagree that the API is quite fragile. Let me abstract this a little better.
experimental/ir/fdp.go
Outdated
| continue | ||
| } | ||
|
|
||
| // HACK: For each optionSpan, check around it for the option keyword and semicolon. |
There was a problem hiding this comment.
Could you explain why this is necessary?
There was a problem hiding this comment.
For file options, for example, the optionSpan doesn't cover the option keyword + ;, but those are needed for parity with protoc. Also, those are also used to grab comments (also parity with protoc).
|
|
||
| // Set of all names that are defined in scope of some message; used for | ||
| // generating synthetic names. | ||
| type syntheticNames intern.Set |
There was a problem hiding this comment.
I think this may want to get moved to fdp? Not sure.
There was a problem hiding this comment.
Unfortunately we cannot, since generateIn requires access to &message.Context().session.intern, and I don't think we want to expose the IR session externally? But not sure... I left it as-is for now.
There was a problem hiding this comment.
Ok, let's do this: add Member.SyntheticOneofName() which returns the name of the corresponding synthetic oneof if there should be one. This nicely makes sure this logic lives in the ir package without exposing this type unnecesarily.
| *p = append(*p, elements...) | ||
| return func() { | ||
| if len(*p) > 0 { | ||
| *p = []int32(*p)[:len(*p)-len(elements)] |
There was a problem hiding this comment.
The []int32 cast is redundant I think.
There was a problem hiding this comment.
Unfortunately no, Go is silly and refuses to operate on the path type, despite the fact that it's clearly a slice.
There was a problem hiding this comment.
p is not a slice, it's a pointer to a slice. You need to do (*p)[...] regardless. Silly thing, tbh.
This implements including
SourceCodeInfowhen loweringthe IR to FDS.
The comment attribution is based on #389, however, instead
of doing comment attribution in the lexer, this is done lazily
when lowered to the FDS.
This PR also provides a couple of fixes for option spans, exposes
Options() iter.Seq[ast.DefOption]for*ast.File, and un-skipsold compiler tests based on source code info.
Lastly, this PR addresses some issues surfaced for the old compiler:
and as a result, source code info was not being populated for reserved
names for editions
to explain it.