Add syntax support for toolchain directive #3633
Conversation
Oh, that's really nice to have some tests for the highlighting. I've added some new ones: 92af479
🤔 I don't see any comments from you besides this one |
syntax/gomod.vim
Outdated
| " * go1.XrcN | ||
| " * go1.X.Y-somesuffix | ||
| " * go1.XrcN-somesuffix more stuff | ||
| syntax match gomodToolchainVersion "go1\.\d\+\(\(.\d\+\)\|\(rc\d\+\)\)\?\([ \t-].*\)\?" contained |
There was a problem hiding this comment.
I don't see any comments from you besides this one
Oof. I must have forgotten to hit Start a review before I submitted. I'll repost the quick summary here:
Two things about this after comparing this expression to the docs.
- From the docs, it doesn't appear that any space separated strings are valid. Did you find something otherwise?
- The docs indicate that a string is valid, but when I check rules, I think the only string that's allowed is
default.
There was a problem hiding this comment.
Here's a link to the regular expression used by Go to validate the toolchain directive: https://cs.opensource.google/go/x/mod/+/refs/tags/v0.14.0:modfile/rule.go;l=309-311
There was a problem hiding this comment.
From the docs, it doesn't appear that any space separated strings are valid. Did you find something otherwise?
I found some more detail digging around the stdlib https://go.googlesource.com/go/+/165383381199de6632665f43561e8e0dfc96f067/src/cmd/go/internal/gover/toolchain.go#24 (from looking for the source of the error 'go: invalid toolchain "%s" in go.mod'
Here's a link to the regular expression used by Go to validate the toolchain directive: https://cs.opensource.google/go/x/mod/+/refs/tags/v0.14.0:modfile/rule.go;l=309-311
More than happy to just use that since it's 1. simpler, 2. public documentation. It will permit versions that aren't accepted by the Go tool, e.g. toolchain go1.2.2rc4 will satisfy that but error go: invalid toolchain "go1.2.2rc4" in go.mod if actually used. I think it's fine if the highlighting accepts some invalid values (as long as it doesn't reject any valid ones)
There was a problem hiding this comment.
Trying this with f4260d8. It's still more restrictive than just matching on ^go1 (trying to follow the examples) let me know if you'd like it relaxed further
There was a problem hiding this comment.
No need to relax it further, and I think it may be appropriate to tighten it up so that we don't match invalid versions.
This should also allow default if I'm not mistaken.
There was a problem hiding this comment.
@matthewhughes934 please feel free to let me know if you'd like to be done with this. I'm happy to take it as is and submit the incremental improvements on top of it myself.
There was a problem hiding this comment.
Good find in stdlib. It looks like my assertion about spaces was wrong: https://go.googlesource.com/go/+/165383381199de6632665f43561e8e0dfc96f067/src/cmd/go/internal/gover/toolchain.go#39.
With that in mind, I think your original regular expression was mostly right except for we should also allow default.
If I'm reading the stdlib source code correctly, then it looks like anything other than default is validated by "internal/gover".IsValid: https://go.googlesource.com/go/+/165383381199de6632665f43561e8e0dfc96f067/src/internal/gover/gover.go#92
There was a problem hiding this comment.
This is a fun tidbit from https://go.dev/doc/toolchain:
The standard Go toolchains are named goV where V is a Go version denoting a beta release, release candidate, or release. For example, go1.21rc1 and go1.21.0 are toolchain names; go1.21 and go1.22 are not (the initial releases are go1.21.0 and go1.22.0), but go1.20 and go1.19 are.
There was a problem hiding this comment.
With that in mind, I think your original regular expression was mostly right except for we should also allow default.
👍 I've done this, and squashed all the changes down into 26f4ff5 there's some reasoning+a summary of this discussion in the commit
please feel free to let me know if you'd like to be done with this. I'm happy to take it as is and submit the incremental improvements on top of it myself.
Go for it
This directive was added to `go.mod` with Go 1.21[1] So that this directive can be highlighted in `go.mod`. It's not entirely clear exactly what the fully supported syntax is. The docs[1] suggests any Go release version, e.g. * `go1.20` * `go1.21.1` * `go1.21.1rc1` `golang.org/x/mod` gives a much more relaxed definition, requiring just that things match against the regex `^default$|^go1($|\.)`[2] Finally there's `FromToolchain` from the stdlib's internals for processing versions[3] which is broader than that from[1] but more limited than that from[2], supporting arbitrary suffixes (after any of `" \t-"`) appended to the version, e.g. * go1.21.3-somesuffix * go1.21rc2-somesuffix * go1.21 some-suffix The approach taken for the syntax matching here is closest to this final condition, and will not include some toolchain verison's that would be supported by the `modfile` regex, e.g. * go1.21.1blah * go1.21!some-suffix Since these would be rejected by the `go` tool itself with an error like > go: invalid toolchain "go1.21.1blah" in go.mod
matthewhughes934
force-pushed
the
add-toolchain-syntax-support
branch
from
f4260d8
to
26f4ff5
Compare
This directive was added to
go.modwith Go 1.21[1] So that thisdirective can be highlighted in
go.mod.It's not entirely clear exactly what the fully supported syntax is. The
docs[1] suggests any Go release version, e.g.
go1.20go1.21.1go1.21.1rc1golang.org/x/modgives a much more relaxed definition, requiring justthat things match against the regex
^default$|^go1($|\.)[2]Finally there's
FromToolchainfrom the stdlib's internals forprocessing versions[3] which is broader than that from[1] but more
limited than that from[2], supporting arbitrary suffixes (after any of
" \t-") appended to the version, e.g.The approach taken for the syntax matching here is closest to this final
condition, and will not include some toolchain verison's that would be
supported by the
modfileregex, e.g.Since these would be rejected by the
gotool itself with an error like