From 24bce887e4f383c868db19eaa48ea9e3730f11f7 Mon Sep 17 00:00:00 2001 From: Joshua Humphries <2035234+jhump@users.noreply.github.com> Date: Fri, 27 Oct 2023 15:09:38 -0400 Subject: [PATCH] Update to match Editions behavior of latest protoc (v25.0) (#191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Between v24 and v25, the implementation or editions inside of `protoc` changed quite rather drastically. Most of the complexity having to do with computation of feature defaults and feature set resolution has been completely removed 🎉. This still does not perform some of the checks for field features that `protoc` performs, so that still needs to come in a subsequent branch. But this does bring this repo up to the same level of `protoc`-compatibility it had before, but this time with `protoc` v25 (instead of v24). It also does add one add'l check: that the `packed` field option is not used with editions. --- .protoc_version | 2 +- compiler.go | 29 +- go.mod | 2 +- go.sum | 4 +- internal/options.go | 13 +- internal/tags.go | 3 + internal/testdata/all.protoset | Bin 35842 -> 36483 bytes internal/testdata/desc_test_complex.protoset | Bin 17452 -> 18093 bytes .../desc_test_proto3_optional.protoset | Bin 11293 -> 11934 bytes .../testdata/descriptor_impl_tests.protoset | Bin 19017 -> 19658 bytes internal/testdata/editions/all.protoset | Bin 3075 -> 1703 bytes .../editions/features_with_overrides.proto | 2 +- linker/linker_test.go | 65 +-- options/features.go | 447 ------------------ options/options.go | 324 +++++-------- parser/result.go | 23 +- parser/validate.go | 91 +++- parser/validate_test.go | 106 ++++- sourceinfo/source_code_info.go | 3 + 19 files changed, 355 insertions(+), 759 deletions(-) delete mode 100644 options/features.go diff --git a/.protoc_version b/.protoc_version index 7a5aa455..a4194455 100644 --- a/.protoc_version +++ b/.protoc_version @@ -1 +1 @@ -24.0-rc2 +25.0-rc2 diff --git a/compiler.go b/compiler.go index b7b1fea4..2f72c4e3 100644 --- a/compiler.go +++ b/compiler.go @@ -141,13 +141,12 @@ func (c *Compiler) Compile(ctx context.Context, files ...string) (linker.Files, h := reporter.NewHandler(c.Reporter) e := executor{ - c: c, - h: h, - s: semaphore.NewWeighted(int64(par)), - cancel: cancel, - sym: &linker.Symbols{}, - resCache: &options.FeaturesResolverCache{}, - results: map[string]*result{}, + c: c, + h: h, + s: semaphore.NewWeighted(int64(par)), + cancel: cancel, + sym: &linker.Symbols{}, + results: map[string]*result{}, } // We lock now and create all tasks under lock to make sure that no @@ -232,12 +231,11 @@ func (r *result) getBlockedOn() []string { } type executor struct { - c *Compiler - h *reporter.Handler - s *semaphore.Weighted - cancel context.CancelFunc - sym *linker.Symbols - resCache *options.FeaturesResolverCache + c *Compiler + h *reporter.Handler + s *semaphore.Weighted + cancel context.CancelFunc + sym *linker.Symbols descriptorProtoCheck sync.Once descriptorProtoIsCustom bool @@ -577,10 +575,9 @@ func (t *task) link(parseRes parser.Result, deps linker.Files, overrideDescripto return nil, err } - interpretOpts := make([]options.InterpreterOption, 1, 2) - interpretOpts[0] = options.WithFeaturesResolverCache(t.e.resCache) + var interpretOpts []options.InterpreterOption if overrideDescriptorProtoRes != nil { - interpretOpts = append(interpretOpts, options.WithOverrideDescriptorProto(overrideDescriptorProtoRes)) + interpretOpts = []options.InterpreterOption{options.WithOverrideDescriptorProto(overrideDescriptorProtoRes)} } optsIndex, err := options.InterpretOptions(file, t.h, interpretOpts...) diff --git a/go.mod b/go.mod index 191980aa..95b732ed 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/google/go-cmp v0.6.0 github.com/stretchr/testify v1.8.4 golang.org/x/sync v0.4.0 - google.golang.org/protobuf v1.31.1-0.20230727123859-6d0a5dbd9500 + google.golang.org/protobuf v1.31.1-0.20231027082548-f4a6c1f6e5c1 ) require ( diff --git a/go.sum b/go.sum index c49f00cf..2e057c91 100644 --- a/go.sum +++ b/go.sum @@ -12,8 +12,8 @@ golang.org/x/sync v0.4.0 h1:zxkM55ReGkDlKSM+Fu41A+zmbZuaPVbGMzvvdUPznYQ= golang.org/x/sync v0.4.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw= -google.golang.org/protobuf v1.31.1-0.20230727123859-6d0a5dbd9500 h1:7y2/WECm0zxQchjsPsWLAajGJ77KApK/0JpIaBOeDvk= -google.golang.org/protobuf v1.31.1-0.20230727123859-6d0a5dbd9500/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I= +google.golang.org/protobuf v1.31.1-0.20231027082548-f4a6c1f6e5c1 h1:fk72uXZyuZiTtW5tgd63jyVK6582lF61nRC/kGv6vCA= +google.golang.org/protobuf v1.31.1-0.20231027082548-f4a6c1f6e5c1/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/internal/options.go b/internal/options.go index b35d1c0e..02088337 100644 --- a/internal/options.go +++ b/internal/options.go @@ -26,15 +26,26 @@ type hasOptionNode interface { FileNode() ast.FileDeclNode // needed in order to query for NodeInfo } +func FindFirstOption(res hasOptionNode, handler *reporter.Handler, scope string, opts []*descriptorpb.UninterpretedOption, name string) (int, error) { + return findOption(res, handler, scope, opts, name, false, true) +} + func FindOption(res hasOptionNode, handler *reporter.Handler, scope string, opts []*descriptorpb.UninterpretedOption, name string) (int, error) { + return findOption(res, handler, scope, opts, name, true, false) +} + +func findOption(res hasOptionNode, handler *reporter.Handler, scope string, opts []*descriptorpb.UninterpretedOption, name string, exact, first bool) (int, error) { found := -1 for i, opt := range opts { - if len(opt.Name) != 1 { + if exact && len(opt.Name) != 1 { continue } if opt.Name[0].GetIsExtension() || opt.Name[0].GetNamePart() != name { continue } + if first { + return i, nil + } if found >= 0 { optNode := res.OptionNode(opt) fn := res.FileNode() diff --git a/internal/tags.go b/internal/tags.go index 7183dbaf..387df7f3 100644 --- a/internal/tags.go +++ b/internal/tags.go @@ -73,6 +73,9 @@ const ( // FileSyntaxTag is the tag number of the syntax element in a file // descriptor proto. FileSyntaxTag = 12 + // FileEditionTag is the tag number of the edition element in a file + // descriptor proto. + FileEditionTag = 14 // MessageNameTag is the tag number of the name element in a message // descriptor proto. MessageNameTag = 1 diff --git a/internal/testdata/all.protoset b/internal/testdata/all.protoset index f10decce25bd020f5a44b3416d4a52e429e0ace1..335713e6e6fe84d158d9b049be792dc7e931ac44 100644 GIT binary patch delta 1294 zcmb7EOHUI~75llJ2)`T)M3hNGpY2JjG-OUhk-T(Yzha-T(S+%Tcg^+K681h9z;T26=S;gVCqE^;!R&rrv zF)~WhE(n2-XX1!gBy=OqNyZ&nf7bU25U=a%;CFIz3HTgh66Z3N)iP`g<@!gcjp6>z zp5L7SusBn$6!9!B!zYCPDjJ`ecuWvA!d)YXmqa)8qJOIc^`x^r&W=L39? zlDcy$Ud^V8xP%MK80PeIqp^{TmY&Db>w=yHFV3Q*rPc}EVhRbN1|@lMp^}$3)-Zgk zmqz8K!DA*EA?eTzkrb1dF-`{YY!R;+Dc~$?hDL>DEvpt*;5zVE%DI?wJF}{0GgkH8 zR!N5SU%_5BoHoF@11xTPv~xCG38H{3heoJ5BysGb8OmoWwn%VYe|VW49cXiU2Ksp< zOv%@jl!RDr!7?oQ^SD&Xtl$H4cXZd-9RniKqR?KJ0`kS$z$pr8nP6hUYU|MEl${fRUNk-;GK}icUf;cbGr=S;D zxzPj!z{!jz_5Jak=(NY%py)m7!afOLYbc!2D!~Qd&DvvD3K0XYp{~fJS;=biLAb>^ zJeAS`4>G`?R|{&sl2@85VK?Q(m5_W6!O^rNGV3jgsYqvRC`iJmD{ax%XSsngH0IKOV0Q@M)9qB;8*jnc@C5f^ap?4{G^fF2}DA)r| zH(y3ZB2l9^FbD$8t4oS(j)^Gni$*scplWtI(uR<*9|W6}3F8}Y1Gl$4N8wl-{QNjP b(FVWR^2G0fz_ONa{m=9t52*U|mG`MXPHuJ~ zqXy^Z$HLn=R0BgjgIrw%n1qD5_YN{T-8%0~enw#4MN8w8YY!5+Nkh8NsHn)sp2Dn9Ie*<(8UQQd*Q6oLVB3H~F@) zlb%cmBjXH4mKltUix>sCSd0vejDc+)adb5a6|Qj1gbl2e7ECuf*g zs7ipfuz|JkK((;=2e|qLair#z=7v-jqzbi8zF-m}Cxv7k=sy13)Z*gA^i|aruWE~kK?;Fj(qL;so3L=&zQK`krVLZQHj_$Uo0_&m`k|Pp zT}onD=xo@ai5oYvFpVMcH@I-^(in;^3_n1PGt=n@5GBs;zVptx=e~RIyxk)9wh1fv zx(3v~YgeC-r^ub2cUzd83IYnJbCsM@^f^a}NoUX($SO)Uj{_SerJ~%eW&)^**ME`w z_R9%GODOKPKnR>19YvfhqJ< z;S2SRUy>$HnV|W&Nz+6UbZlOWG~$I4-q1?GX+}?tit9=`SIojq;Hp+KVflV4pG&8V z*SD%y96nX?1C-Q|r8W?48f+GgpA8TPb1OGspyRXvYWm+O$=rut!wxiQjZ^E`BM zh@X>g$q5lL?26%5bQN&9oXX<1vq$R8`0UM)LA2};&3Exa$e}qpwkk)_q9h}R5ya+; z1K2}zoUp8W0QbsDkx@#8R7L$VeiV-Z=kZeD5b!1?CM++}92=)4R^a6%KhF!xy!#xq z1EV%K0Rb>8bCYUtVn1}-W^YjLLu&Ll31DX;%(*JV6<|*{XRPcaMl6%P!C7@-Y^c_{ z5a@7@kEGbN~UMW;zHH4TZ>W=Eo??Nra18cl|&ap@@me1CKM z9}6VvOKsKXN8x$M> zeXX0p>0n6f4fFy}>*%^H>0=`B+^W`16R7Fk4t60V90uMNC8B*ZUEuDv?IaxTf?uA7 YBVF*TZCmsS@T@C^&d;p=VCwzgADU8xAOHXW delta 670 zcmZvaJ#W)M9L9TzDbA4>b8ScpRD`_nA^}2W0I4A1)aTZ!dv+bCEn+ZMa;c0G2gMEu zp-L=BSu&toGJuenK>{*lWMk(eAck&;Pk=CvRU*W8y8r#2d!Fa+{Sdqx01keyLpHp6 z{$<7He{pa@cG{GE;L9IBt;U2rJWiXA?{wXKssJt}#<}=}FKpKp14}{-A(&RhDpoB6 z>sT~{y?C(!7ui>Ck$vW~<6QC$1Z+Gx^&?ii3)6TiCDE4MZ~JIQ9E9TEmX;I3YY3rC zZNJ~6HR>amWlGg+X9oa`05<~QEf65La((3n;_;rYDr<^qq*@MbH}xL%sN10EK6_QV zRmq3Qe263?gwu3P44%6Ej`{QvMK2gvHqV@%5l=@a=XG&y6HDxQIlnvrFz5L_$KAKW zi>$|X+iBWiD;z77i(9pDD*th@b2O_4;F0N*k#1s3B$8-q#yulD)%=G-&)7mmzFC@K za}k?SF;DYNnx(bxT&T zDp@`B#kMykEb z!`8u{Rsa~BELVzn376puLj61%UI=XwM2#@j2;vPf04*pHG=A#Bp6N%B01ufEu7mIj9D*>mNW@=PeS5nzR25tgJrJN4R50klUDyhG| zNxf$EsEQY$xP~lufN;xTt6Ka4TAMtW>sfDR7v zbJ86-CL)GS=x#+v9+yhV4DL94qRx!Z-VW%*z8cYb7dM0)nxiADau_X1GGZ7(Y`r*u z{WQl3D@_mJK}jhvN->`-t6#>Cq7h&}UJ4un?zqGR&`uviVA0ZtsKX-$N?e4T7PmOC&eD5>;)d`p?v8F$|3>&k5l9o7?|b zAhEgBrsiC!N5i4|w~41@b?Q3-sJ2%ZaG)rAq602%WsSp_C`f|ec|gIFhXH4Uf`j0E z`=)=|AJBRO-N4m8x+zP|F%fufRqLh!RGZ!Q_aG!32JSW`qFDIVF0QO&iM_1k3wr?OJb&Q0J63p+ z_0;YKQ6Y8X0-r3GJP`AO>Brn5>3sxZ)B&M|4`@!Td2yn$}?;( zVlyfxtLB=e8(3DFSTZs_>Us7K#cr$L4(uVy-c;`_Ps@srz%+a+!jFlns%;zavDbIy z{-9_3NP@|M{mkl2>-O>oY_m{8wE&$nP6U(a9Xyt}`0J4cv;WPB%J6*MG!(LG$qH5_ zYfDs>AfuuYt3lR@woQaNl!AYw3jpzuTT_S`{Gnh)G=haVyL)Le-o3IL8y{SqfZvij By(jiBrKeE?!d@6Q- zg$qp#8^;AM)I<|^CMGrEH)!0sa&IVcVf+EaIIli{pmBEho_o%BzSn$u4nDsCAFd8k z-`9+5gO8})j_SujLXAcsP14y?R?ml+Au!BDLg95?U*90%&4OOi@02rP?BLar3wz%F z1ZGvt`P&eJjL62Ys3~}UQBce~s(Lu`1%mh0PZK|>={3l>M2=*V<&6?*3Dx$GP~Rp8 zI(rT~A>>K2R4$MOQbJFG;bCKQQ%?Zc1?{^4yaC>}zx{xO>$_tpY)jCnToWN43Mk`*DFXz;In*=>G zzOw4tl@lgdiF0YDNP>;enJ1IjTOgZe3M9>2p>aN?r?dHWv<-dbQYNZxB{#C^q+R`X zyCkE={N+AAyl8?;4J_t(baFObNwS1(hi0fHtO)$76^gMGn{aFcU% zDwPHgvCx;x=CiqSPOGnk)09(JLSbBjt7%Ilx0@0bbFDi5KI^82!*DguKL#*#n%n=_ zA;sG3c5AeZ{^_Y|FaLn5jQ#+Su6qqYALfK(9SE3P>s}@lS(PLHeHtCV9QW5~)C14f zUq;3v<7RJA5C-a3*EQ7|6Il{h&2G9t#p-sX1)Au?ZIiJ{w^L(-*l1D? zhdi0Ur3Y`~ClJ@oQiFIK=AGx6-}}ydc?~|l0v|3{GDo}Y zYUM@l7YFARw@cYh_2TebWj(`};WTZ#k?VWq!UeFBA4=6xZh2={C0OP&2*IKzh*+}; zHnCvEd)ZnGo;Ez{cM^$o=`93ocy#?orgj$=@z^EPj??c(Xu=K>dtq%c&%c2XD%6Sk z0d3Lm*0Le$RUPh@zLQE?Skd6Y^qua>^8>Ei98#z`n|*#X|rW!YFXP13S6P ze(ZGJwv$YTW13joZYEXbPmsOinizw}x#v=Kep}FG!7|8wQW`t|q0lpSM^bJsO|T0o zJF8*6VQtzb!HT*c=bP=(Fmw(m_F8^Bj)y4wC~a(v=an6RasP!BKPhOMagV@d_I;xV$E zRQAr50~dIPeF0v8hoN{0=o$HEwS)qldb+>qZuNX$!=D;hQJRJ^okWW?4<|Ir7a5(m zW*N&FsZ1Dajian_2uhDJ>lrAWC?f_a(fGoY_N4u)$IZqb54Ez0w3ZmQ_6wLfSr`)rBSEt|)jX`bDC1&(|Q+B}bPYHFCf-rn6@V9e)%qb;vM zkWYBc>^4fxZGqicW4~odjJRc(duZx#4xMey)0$*)3oOS3GA*L<(7cr=b)HGWbA>2W}u zl$`8pM|&;Fx+1vwq!0loYqju8LJRy^QBKHMXM+o*;Q!6T1}F4oUOo8wDRQ##3oaO&;HrLj2ow98Uf%tn_yfyCKsS#o)mi--EJD+QE11GtN> zX;|zM6<@gCwo0||30h|e5jym$94*YocTipX$u8^!?y);Oa|?~4I(MMzIVYh%aGXJz{t``j6zngO<7qJW2IDBubx9@nf$G_bqJMujJ`Uj(+HbL-Hv tUm8_QeJ&ev`|FahaYG(3pWV>>Dw$rAGFg$});~O*%Fl|b%Z5}^?%z`#w^0B9 literal 3075 zcmbVO&2HmV6t4eT=S1x|w}3K+flkB>C5vbjBs3~wMo#K>N)x9z4OP2|oW^ZEb>oiX zwCe1502a&!!Is@@*t6sTK;jvA0Nw!3y>@==-c*8YBH#0!|8u`{;co!l@%?V!J#j{U z)ID*XXcW4`%4HZt0a-c;f=bT`OMeI;Z3Th;BmUQ%kbDf1(>pE-Yr=Z2M&1hdR6!Hi zr|?Ah_1S_=#ic_Eeoa0CwPqZ3k4(FzE$9W31(Oe9i2_=dsRYQv^1!)p??{HpNeM)+UlcKAnVT|7Nf{P>r|0^^qO>N> zDNTYPbx@or3Wr4*q510lN>c0!3#(#KnK_8U8?LulmwXAzFmfWds9;8}8$xzfoxcW>@!1{ITaP%!9`%i6)8z!IOU+uX~MM%o?AqDYZ*jmW1!rZ(%X=3c|xH#La|{RFfrbj#Rjn46l+1D0WR3alGe z-E2^-!o#MF?irmn0v93fASFWw8vK9q3f+Bknwncgy_d?YDhU5T%# zIBO318^zSx>6yp)NsX;h6gAFRUMz|>1z|7<|IW~s+?viJ~G8qY0vt%0;uEAILw$v+l(1EW_|KX z$+mSy^Ntu8SEk{mzohsTz}cK>q#bY0=OFv;NyH{+rTT8FyXK8s5Bwl$@vUiPia20O z=$)QLY+(PRg;;Mzhvw?6miH>Is2o zU>|^D92mQ1!rEymBiQs2r_JR*adC~?L*Hmgij&aAnFk+1hcz1ukgOh2OG$BO)*7Ah zvdIV^fwFCzrltKe{>87r3=BQIFpkW|B&C#x2e5dEyQg)K?4FFW4w5}tHyIfXyKx&=Z5(Pjj!<9SD5xo8_)$Gg?J;~pZ3gY&!i~m*)h>E^||D1BCY?~SyI?q^=*GX2c%qkjRN`d jkl = 5 [features.string_field_validation = NONE]; + map jkl = 5 [features.utf8_validation = NONE]; Bar mno = 6 [features.message_encoding = DELIMITED]; repeated double pqr = 7; map stu = 8; diff --git a/linker/linker_test.go b/linker/linker_test.go index 589e7f77..31188a18 100644 --- a/linker/linker_test.go +++ b/linker/linker_test.go @@ -2025,9 +2025,6 @@ func TestLinkerValidation(t *testing.T) { `, }, expectedErr: `test.proto:1:11: edition value "2024" not recognized; should be one of ["2023"]`, - // protoc v24.0-rc2 doesn't (yet?) reject unrecognized editions that - // sort *after* 2023; only ones before 2023 🤷 - expectedDiffWithProtoc: true, }, "failure_unknown_edition_past": { input: map[string]string{ @@ -2041,44 +2038,6 @@ func TestLinkerValidation(t *testing.T) { }, expectedErr: `test.proto:1:11: edition value "2022" not recognized; should be one of ["2023"]`, }, - "failure_use_of_features_without_editions": { - input: map[string]string{ - "test.proto": ` - syntax = "proto3"; - message Foo { - string foo = 1 [features.field_presence = LEGACY_REQUIRED]; - int32 bar = 2 [features.field_presence = IMPLICIT]; - } - `, - }, - expectedErr: `test.proto:3:25: field Foo.foo: option 'features' may only be used with editions but file uses "proto3" syntax`, - }, - "failure_direct_use_of_raw_features": { - input: map[string]string{ - "test.proto": ` - edition = "2023"; - message Foo { - string foo = 1 [features.raw_features.field_presence = LEGACY_REQUIRED]; - } - `, - }, - expectedErr: `test.proto:3:34: feature field "raw_features" may not be used explicitly`, - expectedDiffWithProtoc: true, // seems like a bug in protoc that it allows use of raw_features - }, - "failure_direct_use_of_raw_features_in_message_literal": { - input: map[string]string{ - "test.proto": ` - edition = "2023"; - message Foo { - string foo = 1 [features = { - raw_features: - }]; - } - `, - }, - expectedErr: `test.proto:4:5: feature field "raw_features" may not be used explicitly`, - expectedDiffWithProtoc: true, // seems like a bug in protoc that it allows use of raw_features - }, "success_proto2_packed": { input: map[string]string{ "test.proto": ` @@ -2250,6 +2209,30 @@ func TestLinkerValidation(t *testing.T) { }, expectedErr: `test.proto:3:3: packed option is only allowed on repeated fields`, }, + "failure_editions_feature_on_wrong_target_type": { + input: map[string]string{ + "test.proto": ` + edition = "2023"; + message Foo { + int32 i32 = 1 [features.enum_type=OPEN]; + } + `, + }, + expectedErr: `test.proto:3:18: feature "enum_type" is allowed on [enum,file], not on field`, + }, + "failure_editions_feature_on_wrong_target_type_msg_literal": { + input: map[string]string{ + "test.proto": ` + edition = "2023"; + message Foo { + int32 i32 = 1 [features={ + enum_type: OPEN + }]; + } + `, + }, + expectedErr: `test.proto:4:5: feature "enum_type" is allowed on [enum,file], not on field`, + }, } for name, tc := range testCases { diff --git a/options/features.go b/options/features.go deleted file mode 100644 index 5cca188f..00000000 --- a/options/features.go +++ /dev/null @@ -1,447 +0,0 @@ -// Copyright 2020-2023 Buf Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package options - -import ( - "errors" - "fmt" - "strings" - "sync" - - "google.golang.org/protobuf/encoding/prototext" - "google.golang.org/protobuf/proto" - "google.golang.org/protobuf/reflect/protoreflect" - "google.golang.org/protobuf/reflect/protoregistry" - "google.golang.org/protobuf/types/descriptorpb" - - "github.com/bufbuild/protocompile/ast" - "github.com/bufbuild/protocompile/internal" - "github.com/bufbuild/protocompile/linker" - "github.com/bufbuild/protocompile/reporter" -) - -const ( - // featuresFieldName is the name of a field in every options message. - featuresFieldName = "features" - // rawFeaturesFieldName is the name of a field in FeatureSet. - rawFeaturesFieldName = "raw_features" -) - -var ( - featureSetType = (*descriptorpb.FeatureSet)(nil).ProtoReflect().Type() - featureSetName = featureSetType.Descriptor().FullName() - - errMissingDefault = errors.New("edition is missing default value") -) - -// FeaturesResolverCache is a cache of features resolvers. An -// individual resolver is identified by a descriptor for the -// google.protobuf.FeatureSet message. So the cache supports -// multiple versions. -type FeaturesResolverCache struct { - mu sync.RWMutex - entries map[protoreflect.MessageType]*FeaturesResolver -} - -// GetResolver returns a FeaturesResolver associated with the given -// descriptor. The given file descriptor must be for the message -// google.protobuf.FeatureSet. An error is returned if the given -// descriptor is not a valid FeatureSet definition. -func (c *FeaturesResolverCache) GetResolver(msgType protoreflect.MessageType) (*FeaturesResolver, error) { - if c == nil { - // If nil, no caching. Just go ahead and create a new one. - if err := validateFeatures(msgType.Descriptor()); err != nil { - return nil, err - } - return &FeaturesResolver{msgType: msgType, defaults: map[string]*featuresDefault{}}, nil - } - - c.mu.RLock() - res := c.entries[msgType] - c.mu.RUnlock() - if res != nil { - return res, nil - } - - if err := validateFeatures(msgType.Descriptor()); err != nil { - return nil, err - } - - c.mu.Lock() - defer c.mu.Unlock() - // double-check in case it was concurrently added - if res := c.entries[msgType]; res != nil { - return res, nil - } - res = &FeaturesResolver{msgType: msgType, defaults: map[string]*featuresDefault{}} - if c.entries == nil { - c.entries = map[protoreflect.MessageType]*FeaturesResolver{} - } - c.entries[msgType] = res - return res, nil -} - -// FeaturesResolver helps to resolve feature values in source -// files that use editions. -type FeaturesResolver struct { - msgType protoreflect.MessageType - mu sync.RWMutex - defaults map[string]*featuresDefault -} - -func (r *FeaturesResolver) GetDefaults(edition string) (protoreflect.Message, error) { - r.mu.RLock() - res := r.defaults[edition] - r.mu.RUnlock() - if res != nil { - res.maybeInit() - return res.features, res.err - } - - res = &featuresDefault{msgType: r.msgType, edition: edition} - r.mu.Lock() - if existing := r.defaults[edition]; existing != nil { - res = existing - } else { - r.defaults[edition] = res - } - r.mu.Unlock() - - res.maybeInit() - return res.features, res.err -} - -type featuresDefault struct { - msgType protoreflect.MessageType - edition string - init sync.Once - - features protoreflect.Message - err error -} - -func (d *featuresDefault) maybeInit() { - d.init.Do(func() { - d.features, d.err = populateFeatureDefaults(d.msgType, d.edition) - }) -} - -func validateFeatures(md protoreflect.MessageDescriptor) error { - if md.FullName() != featureSetName { - return reporter.Errorf(posOf(md), "message name should be %q but instead is %q", featureSetName, md.FullName()) - } - if md.Oneofs().Len() > 0 { - ood := md.Oneofs().Get(0) - return reporter.Errorf(posOf(ood), "FeatureSet has oneof (%s): editions does not support oneof features", ood.Name()) - } - fields := md.Fields() - for i, length := 0, fields.Len(); i < length; i++ { - fld := fields.Get(i) - switch fld.Cardinality() { - case protoreflect.Required: - return reporter.Errorf(posOf(fld), "FeatureSet has required field (%s): editions does not support required features", fld.Name()) - case protoreflect.Repeated: - return reporter.Errorf(posOf(fld), "FeatureSet has repeated field (%s): editions does not support repeated or map features", fld.Name()) - } - opts, ok := fld.Options().(*descriptorpb.FieldOptions) - if !ok { - return reporter.Errorf(posOf(fld), "FeatureSet field %s: options have incorrect type (%T)", fld.Name(), fld.Options()) - } - if len(opts.Targets) == 0 { - return reporter.Errorf(posOf(fld), "FeatureSet field %s is missing target types", fld.Name()) - } - } - return nil -} - -func posOf(desc protoreflect.Descriptor) ast.SourcePos { - fd := desc.ParentFile() - // First see if we can get the source location from the AST. - if result, ok := fd.(linker.Result); ok { - if msg := findProto(result.FileDescriptorProto(), desc); msg != nil { - node := result.Node(msg) - if node != nil { - return result.FileNode().NodeInfo(node).Start() - } - } - srcLocs := result.FileDescriptorProto().GetSourceCodeInfo().GetLocation() - if fd.SourceLocations().Len() == 0 && len(srcLocs) > 0 { - // We haven't yet built the protoreflect.SourceLocations index for this - // result, so we have to trawl through the proto. - path, ok := internal.ComputePath(desc) - if ok { - for _, srcLoc := range srcLocs { - if pathsEqual(srcLoc.Path, path) { - return ast.SourcePos{ - Filename: fd.Path(), - Line: int(srcLoc.Span[0] + 1), - Col: int(srcLoc.Span[1] + 1), - } - } - } - } - return ast.UnknownPos(fd.Path()) - } - } - // If not, try to get it from the descriptor's source locations. - srcLoc := fd.SourceLocations().ByDescriptor(desc) - if internal.IsZeroLocation(srcLoc) { - return ast.UnknownPos(fd.Path()) - } - return ast.SourcePos{ - Filename: fd.Path(), - Line: srcLoc.StartLine + 1, - Col: srcLoc.StartColumn + 1, - } -} - -func pathsEqual(a, b []int32) bool { - if len(a) != len(b) { - return false - } - for i := range a { - if a[i] != b[i] { - return false - } - } - return true -} - -func findProto(fd *descriptorpb.FileDescriptorProto, element protoreflect.Descriptor) proto.Message { - if element == nil { - return nil - } - if _, ok := element.(protoreflect.FileDescriptor); ok { - return fd - } - parent := findProto(fd, element.Parent()) - if parent == nil { - return nil - } - switch e := element.(type) { - case protoreflect.MessageDescriptor: - switch p := parent.(type) { - case *descriptorpb.FileDescriptorProto: - return p.MessageType[e.Index()] - case *descriptorpb.DescriptorProto: - return p.NestedType[e.Index()] - } - case protoreflect.FieldDescriptor: - switch p := parent.(type) { - case *descriptorpb.FileDescriptorProto: - if e.IsExtension() { - return p.Extension[e.Index()] - } - case *descriptorpb.DescriptorProto: - if e.IsExtension() { - return p.Extension[e.Index()] - } - return p.Field[e.Index()] - } - case protoreflect.OneofDescriptor: - if p, ok := parent.(*descriptorpb.DescriptorProto); ok { - return p.OneofDecl[e.Index()] - } - case protoreflect.EnumDescriptor: - switch p := parent.(type) { - case *descriptorpb.FileDescriptorProto: - return p.EnumType[e.Index()] - case *descriptorpb.DescriptorProto: - return p.EnumType[e.Index()] - } - case protoreflect.EnumValueDescriptor: - if p, ok := parent.(*descriptorpb.EnumDescriptorProto); ok { - return p.Value[e.Index()] - } - case protoreflect.ServiceDescriptor: - if p, ok := parent.(*descriptorpb.FileDescriptorProto); ok { - return p.Service[e.Index()] - } - case protoreflect.MethodDescriptor: - if p, ok := parent.(*descriptorpb.ServiceDescriptorProto); ok { - return p.Method[e.Index()] - } - } - // Shouldn't get here... - return nil -} - -func populateFeatureDefaults(msgType protoreflect.MessageType, edition string) (protoreflect.Message, error) { - defaults := msgType.New() - // TODO: how to handle extensions? - var missing []string - fields := msgType.Descriptor().Fields() - for i, length := 0, fields.Len(); i < length; i++ { - fld := fields.Get(i) - if fld.Name() == rawFeaturesFieldName { - // set to empty message - defaults.Set(fld, defaults.NewField(fld)) - continue - } - err := setEditionDefault(defaults, fld, edition) - if err == errMissingDefault { - missing = append(missing, string(fld.Name())) - continue - } else if err != nil { - return nil, fmt.Errorf("edition %q is not correctly configured: failed to compute default for feature %s: %w", edition, fld.Name(), err) - } - } - if len(missing) > 0 { - return nil, fmt.Errorf("edition %q is not configured: could not compute defaults for features [%s]", edition, strings.Join(missing, ",")) - } - return defaults, nil -} - -func setEditionDefault(msg protoreflect.Message, fld protoreflect.FieldDescriptor, edition string) error { - opts, ok := fld.Options().(*descriptorpb.FieldOptions) - if !ok { - return errMissingDefault - } - // Editions inherit defaults from prior editions, so the defaults can be sparse. - // So we have to search for the greatest edition that is less than or equal to - // the given one, and use the default value associated with that. - var matched bool - var maxMatch string - var value *string - // TODO: We do dumb linear scan since defaults are not guaranteed to be - // sorted. A possible optimization would be to sort the slice once, when - // the FeaturesResolver is initialized, and then binary search. - for _, def := range opts.EditionDefaults { - defEdition := def.GetEdition() - if defEdition == edition || editionLess(defEdition, edition) { - if !matched || editionLess(maxMatch, defEdition) { - maxMatch = defEdition - matched = true - value = def.Value - } - } - } - if !matched || value == nil { - return errMissingDefault - } - valStr := *value - // We use a typed nil so that it won't fall back to the global registry. Features - // should not use extensions or google.protobuf.Any, so a nil *Types is fine. - unmarshaler := prototext.UnmarshalOptions{Resolver: (*protoregistry.Types)(nil)} - // The string value is in the text format: either a field value literal or a - // message literal. (Repeated and map features aren't supported, so there's no - // array or map literal syntax to worry about.) - if fld.Kind() == protoreflect.MessageKind || fld.Kind() == protoreflect.GroupKind { - fldVal := msg.NewField(fld) - err := unmarshaler.Unmarshal([]byte(valStr), fldVal.Message().Interface()) - if err != nil { - return err - } - msg.Set(fld, fldVal) - return nil - } - // The value is the textformat for the field. But prototext doesn't provide a way - // to unmarshal a single field value. To work around, we unmarshal into the enclosing - // message, so we prefix the value with the field name. - valStr = fmt.Sprintf("%s: %s", fld.Name(), valStr) - // Sadly, prototext doesn't support a Merge option. So we can't merge the value - // directly into the supplied msg. We have to instead unmarshal into an empty - // message and then use that to set the field in the supplied msg. :( - empty := msg.Type().New() - err := unmarshaler.Unmarshal([]byte(valStr), empty.Interface()) - if err != nil { - return err - } - msg.Set(fld, empty.Get(fld)) - return nil -} - -// editionLess returns true if a is less than b. This custom sort order was -// transliterated from the C++ implementation in protoc: -// -// https://github.com/protocolbuffers/protobuf/blob/v24.0-rc2/src/google/protobuf/feature_resolver.cc#L78 -func editionLess(a, b string) bool { - aParts := strings.Split(a, ".") - bParts := strings.Split(b, ".") - for i := 0; i < len(aParts) && i < len(bParts); i++ { - if len(aParts[i]) != len(bParts[i]) { - return len(aParts[i]) < len(bParts[i]) - } - if aParts[i] != bParts[i] { - return aParts[i] < bParts[i] - } - } - // Both strings are equal up until an extra element, which makes that string - // more recent. - return len(aParts) < len(bParts) -} - -func resolveFeatures(features, defaults protoreflect.Message, res linker.Resolver) (protoreflect.Message, error) { - resolved := proto.Clone(defaults.Interface()) - if features.Descriptor() == defaults.Descriptor() { - // Same descriptor means we can do this the easy way. - // Merge the specified features on top of the defaults. - proto.Merge(resolved, features.Interface()) - // And then try to set the raw_features field to the original - rawField := defaults.Descriptor().Fields().ByName(rawFeaturesFieldName) - if rawField != nil && rawField.Message() == features.Descriptor() { - if features.IsValid() { - resolved.ProtoReflect().Set(rawField, protoreflect.ValueOfMessage(features)) - } else { - // features is invalid if it is a nil pointer, which means unset, - // so also clear the raw field to follow suit - resolved.ProtoReflect().Clear(rawField) - } - } - return resolved.ProtoReflect(), nil - } - // Descriptors don't match, which means we need to merge values by - // serializing to bytes and then de-serializing. - data, err := proto.Marshal(features.Interface()) - if err != nil { - return nil, err - } - rawFeatures := defaults.New() - if err := (proto.UnmarshalOptions{Resolver: res}).Unmarshal(data, rawFeatures.Interface()); err != nil { - return nil, err - } - proto.Merge(resolved, rawFeatures.Interface()) - rawField := defaults.Descriptor().Fields().ByName(rawFeaturesFieldName) - if rawField != nil && rawField.Message() == defaults.Descriptor() { - resolved.ProtoReflect().Set(rawField, protoreflect.ValueOfMessage(rawFeatures)) - } - return resolved.ProtoReflect(), nil -} - -func setFeatures(options, features protoreflect.Message, res linker.Resolver) error { - fld := options.Descriptor().Fields().ByName(featuresFieldName) - if fld == nil { - // no features to resolve - return nil - } - if fld.IsList() || fld.Message() == nil || fld.Message().FullName() != featureSetName { - // features field doesn't look right... abort - // TODO: should this return an error? - return nil - } - if fld.Message() == features.Descriptor() { - options.Set(fld, protoreflect.ValueOfMessage(features)) - return nil - } - // Descriptors don't match, which means we need to set the value by - // serializing to bytes and then de-serializing. - dest := options.NewField(fld) - options.Set(fld, dest) - data, err := proto.Marshal(features.Interface()) - if err != nil { - return err - } - return proto.UnmarshalOptions{Resolver: res}.Unmarshal(data, dest.Message().Interface()) -} diff --git a/options/options.go b/options/options.go index 77d91ca0..c7645acf 100644 --- a/options/options.go +++ b/options/options.go @@ -49,12 +49,21 @@ import ( "github.com/bufbuild/protocompile/sourceinfo" ) +const ( + // featuresFieldName is the name of a field in every options message. + featuresFieldName = "features" +) + +var ( + featureSetType = (*descriptorpb.FeatureSet)(nil).ProtoReflect().Type() + featureSetName = featureSetType.Descriptor().FullName() +) + type interpreter struct { file file resolver linker.Resolver container optionsContainer overrideDescriptorProto linker.File - featuresResolverCache *FeaturesResolverCache lenient bool reporter *reporter.Handler index sourceinfo.OptionIndex @@ -88,14 +97,6 @@ func WithOverrideDescriptorProto(f linker.File) InterpreterOption { } } -// WithFeaturesResolverCache returns an option that indicates the resolver cache to -// use, for resolving features of source files that use editions. -func WithFeaturesResolverCache(cache *FeaturesResolverCache) InterpreterOption { - return func(interp *interpreter) { - interp.featuresResolverCache = cache - } -} - // InterpretOptions interprets options in the given linked result, returning // an index that can be used to generate source code info. This step mutates // the linked result's underlying proto to move option elements out of the @@ -148,67 +149,42 @@ func interpretOptions(lenient bool, file file, res linker.Resolver, handler *rep opt(&interp) } - var featuresDefaults protoreflect.Message - if interp.file.FileDescriptorProto().Edition != nil { - msgType := featureSetType - if md := interp.resolveOptionsType(string(featureSetName)); md != nil { - // use override version - msgType = dynamicpb.NewMessageType(md) - } - res, err := interp.featuresResolverCache.GetResolver(msgType) - if err != nil { - if err := interp.reporter.HandleError(err); err != nil { - return nil, err - } - } - edition := interp.file.FileDescriptorProto().GetEdition() - featuresDefaults, err = res.GetDefaults(edition) - if err != nil { - node := editionNode(interp.file) - if err := interp.reporter.HandleErrorWithPos(interp.nodeInfo(node).Start(), err); err != nil { - return nil, err - } - // if we are proceeding despite the error, use an empty set of defaults - featuresDefaults = msgType.New() - } - } - fd := file.FileDescriptorProto() prefix := fd.GetPackage() if prefix != "" { prefix += "." } - featuresDefaults, err := interpretElementOptions(&interp, fd.GetName(), targetTypeFile, featuresDefaults, fd) + err := interpretElementOptions(&interp, fd.GetName(), targetTypeFile, fd) if err != nil { return nil, err } for _, md := range fd.GetMessageType() { fqn := prefix + md.GetName() - if err := interp.interpretMessageOptions(fqn, md, featuresDefaults); err != nil { + if err := interp.interpretMessageOptions(fqn, md); err != nil { return nil, err } } for _, fld := range fd.GetExtension() { fqn := prefix + fld.GetName() - if err := interp.interpretFieldOptions(fqn, fld, featuresDefaults); err != nil { + if err := interp.interpretFieldOptions(fqn, fld); err != nil { return nil, err } } for _, ed := range fd.GetEnumType() { fqn := prefix + ed.GetName() - if err := interp.interpretEnumOptions(fqn, ed, featuresDefaults); err != nil { + if err := interp.interpretEnumOptions(fqn, ed); err != nil { return nil, err } } for _, sd := range fd.GetService() { fqn := prefix + sd.GetName() - featuresDefaults, err := interpretElementOptions(&interp, fqn, targetTypeService, featuresDefaults, sd) + err := interpretElementOptions(&interp, fqn, targetTypeService, sd) if err != nil { return nil, err } for _, mtd := range sd.GetMethod() { mtdFqn := fqn + "." + mtd.GetName() - _, err := interpretElementOptions(&interp, mtdFqn, targetTypeMethod, featuresDefaults, mtd) + err := interpretElementOptions(&interp, mtdFqn, targetTypeMethod, mtd) if err != nil { return nil, err } @@ -269,82 +245,88 @@ func (interp *interpreter) nodeInfo(n ast.Node) ast.NodeInfo { return interp.file.FileNode().NodeInfo(n) } -func (interp *interpreter) interpretMessageOptions(fqn string, md *descriptorpb.DescriptorProto, featuresDefaults protoreflect.Message) error { - featuresDefaults, err := interpretElementOptions(interp, fqn, targetTypeMessage, featuresDefaults, md) +func (interp *interpreter) interpretMessageOptions(fqn string, md *descriptorpb.DescriptorProto) error { + err := interpretElementOptions(interp, fqn, targetTypeMessage, md) if err != nil { return err } - var mapFieldFeatures map[string]protoreflect.Message for _, fld := range md.GetField() { fldFqn := fqn + "." + fld.GetName() - if err := interp.interpretFieldOptions(fldFqn, fld, featuresDefaults); err != nil { + if err := interp.interpretFieldOptions(fldFqn, fld); err != nil { return err } - if featuresDefaults != nil && fld.GetLabel() == descriptorpb.FieldDescriptorProto_LABEL_REPEATED && - fld.GetType() == descriptorpb.FieldDescriptorProto_TYPE_MESSAGE && - fld.GetTypeName() == "."+fqn+"."+internal.InitCap(internal.JSONName(fld.GetName()))+"Entry" { - // This is a map field, so save the resolved features, so we can apply them to the fields - // in the generated map entry message. - if mapFieldFeatures == nil { - mapFieldFeatures = map[string]protoreflect.Message{} - } - mapFieldFeatures[string(protoreflect.FullName(fld.GetTypeName()).Name())] = fld.GetOptions().GetFeatures().ProtoReflect() - } } for _, ood := range md.GetOneofDecl() { oodFqn := fqn + "." + ood.GetName() - _, err := interpretElementOptions(interp, oodFqn, targetTypeOneof, featuresDefaults, ood) + err := interpretElementOptions(interp, oodFqn, targetTypeOneof, ood) if err != nil { return err } } for _, fld := range md.GetExtension() { fldFqn := fqn + "." + fld.GetName() - if err := interp.interpretFieldOptions(fldFqn, fld, featuresDefaults); err != nil { + if err := interp.interpretFieldOptions(fldFqn, fld); err != nil { return err } } for _, er := range md.GetExtensionRange() { erFqn := fmt.Sprintf("%s.%d-%d", fqn, er.GetStart(), er.GetEnd()) - _, err := interpretElementOptions(interp, erFqn, targetTypeExtensionRange, featuresDefaults, er) + err := interpretElementOptions(interp, erFqn, targetTypeExtensionRange, er) if err != nil { return err } } for _, nmd := range md.GetNestedType() { nmdFqn := fqn + "." + nmd.GetName() - if err := interp.interpretMessageOptions(nmdFqn, nmd, featuresDefaults); err != nil { + if err := interp.interpretMessageOptions(nmdFqn, nmd); err != nil { return err } - if featuresDefaults != nil && nmd.GetOptions().GetMapEntry() { - // This is a synthetic map entry whose fields need the features - // that were defined on the corresponding map field. - mapFeatures := mapFieldFeatures[nmd.GetName()] - if mapFeatures != nil { - for _, fld := range nmd.GetField() { - if fld.Options == nil { - fld.Options = &descriptorpb.FieldOptions{} - } - if err := setFeatures(fld.Options.ProtoReflect(), mapFeatures, interp.resolver); err != nil { - node := interp.file.MessageNode(nmd) - if err := interp.reporter.HandleErrorWithPos(interp.file.FileNode().NodeInfo(node).Start(), err); err != nil { - return err - } - } - } - } - } } for _, ed := range md.GetEnumType() { edFqn := fqn + "." + ed.GetName() - if err := interp.interpretEnumOptions(edFqn, ed, featuresDefaults); err != nil { + if err := interp.interpretEnumOptions(edFqn, ed); err != nil { return err } } + + // We also copy features for map fields down to their synthesized key and value fields. + for _, fld := range md.GetField() { + entryName := internal.InitCap(internal.JSONName(fld.GetName())) + "Entry" + if fld.GetLabel() != descriptorpb.FieldDescriptorProto_LABEL_REPEATED || + fld.GetType() != descriptorpb.FieldDescriptorProto_TYPE_MESSAGE && + fld.GetTypeName() != "."+fqn+"."+entryName { + // can't be a map field + continue + } + if fld.Options == nil || fld.Options.Features == nil { + // no features to propagate + continue + } + for _, nmd := range md.GetNestedType() { + if nmd.GetName() == entryName { + // found the entry message + if !nmd.GetOptions().GetMapEntry() { + break // not a map + } + for _, mapField := range nmd.Field { + if mapField.Options == nil { + mapField.Options = &descriptorpb.FieldOptions{} + } + features := proto.Clone(fld.Options.Features).(*descriptorpb.FeatureSet) //nolint:errcheck + if mapField.Options.Features != nil { + proto.Merge(features, mapField.Options.Features) + } + mapField.Options.Features = features + } + break + } + } + } + return nil } -func (interp *interpreter) interpretFieldOptions(fqn string, fld *descriptorpb.FieldDescriptorProto, featuresDefaults protoreflect.Message) error { +func (interp *interpreter) interpretFieldOptions(fqn string, fld *descriptorpb.FieldDescriptorProto) error { opts := fld.GetOptions() // First process pseudo-options @@ -354,16 +336,15 @@ func (interp *interpreter) interpretFieldOptions(fqn string, fld *descriptorpb.F } } - if len(opts.GetUninterpretedOption()) == 0 && featuresDefaults == nil { - // If no options to interpret and no features to resolve, clear out options. + // Must re-check length of uninterpreted options since above step could remove some. + if len(opts.GetUninterpretedOption()) == 0 { + // If no options to interpret, clear out options. fld.Options = nil return nil } // Then process actual options. - // (Must re-check length of uninterpreted options since above step could remove some.) - _, err := interpretElementOptions(interp, fqn, targetTypeField, featuresDefaults, fld) - return err + return interpretElementOptions(interp, fqn, targetTypeField, fld) } func (interp *interpreter) interpretFieldPseudoOptions(fqn string, fld *descriptorpb.FieldDescriptorProto, opts *descriptorpb.FieldOptions) error { @@ -520,14 +501,14 @@ func encodeDefaultBytes(b []byte) string { return buf.String() } -func (interp *interpreter) interpretEnumOptions(fqn string, ed *descriptorpb.EnumDescriptorProto, featuresDefaults protoreflect.Message) error { - featuresDefaults, err := interpretElementOptions(interp, fqn, targetTypeEnum, featuresDefaults, ed) +func (interp *interpreter) interpretEnumOptions(fqn string, ed *descriptorpb.EnumDescriptorProto) error { + err := interpretElementOptions(interp, fqn, targetTypeEnum, ed) if err != nil { return err } for _, evd := range ed.GetValue() { evdFqn := fqn + "." + evd.GetName() - _, err := interpretElementOptions(interp, evdFqn, targetTypeEnumValue, featuresDefaults, evd) + err := interpretElementOptions(interp, evdFqn, targetTypeEnumValue, evd) if err != nil { return err } @@ -851,32 +832,18 @@ func interpretElementOptions[Elem elementType[OptsStruct, Opts], OptsStruct any, interp *interpreter, fqn string, target *targetType[Elem, OptsStruct, Opts], - featuresDefaults protoreflect.Message, elem Elem, -) (protoreflect.Message, error) { +) error { opts := elem.GetOptions() uo := opts.GetUninterpretedOption() if len(uo) > 0 { - newDefaults, remain, err := interp.interpretOptions(fqn, target.t, featuresDefaults, elem, opts, uo) + remain, err := interp.interpretOptions(fqn, target.t, elem, opts, uo) if err != nil { - return featuresDefaults, err + return err } target.setUninterpretedOptions(opts, remain) - return newDefaults, nil } - if featuresDefaults != nil && opts.GetFeatures() == nil { - if opts == nil { - opts = new(OptsStruct) - target.setOptions(elem, opts) - } - if err := setFeatures(opts.ProtoReflect(), featuresDefaults, interp.resolver); err != nil { - node := interp.file.Node(elem) - if err := interp.reporter.HandleErrorWithPos(interp.file.FileNode().NodeInfo(node).Start(), err); err != nil { - return featuresDefaults, err - } - } - } - return featuresDefaults, nil + return nil } // interpretOptions processes the options in uninterpreted, which are interpreted as fields @@ -887,10 +854,9 @@ func interpretElementOptions[Elem elementType[OptsStruct, Opts], OptsStruct any, func (interp *interpreter) interpretOptions( fqn string, targetType descriptorpb.FieldOptions_OptionTargetType, - featuresDefaults protoreflect.Message, element, opts proto.Message, uninterpreted []*descriptorpb.UninterpretedOption, -) (protoreflect.Message, []*descriptorpb.UninterpretedOption, error) { +) ([]*descriptorpb.UninterpretedOption, error) { optsDesc := opts.ProtoReflect().Descriptor() optsFqn := string(optsDesc.FullName()) var msg protoreflect.Message @@ -899,7 +865,7 @@ func (interp *interpreter) interpretOptions( dm := dynamicpb.NewMessage(md) if err := cloneInto(dm, opts, nil); err != nil { node := interp.file.Node(element) - return nil, nil, interp.reporter.HandleError(reporter.Error(interp.nodeInfo(node).Start(), err)) + return nil, interp.reporter.HandleError(reporter.Error(interp.nodeInfo(node).Start(), err)) } msg = dm } else { @@ -923,18 +889,7 @@ func (interp *interpreter) interpretOptions( } // uninterpreted_option might be found reflectively, but is not actually valid for use if err := interp.reporter.HandleErrorf(interp.nodeInfo(node.GetName()).Start(), "%vinvalid option 'uninterpreted_option'", mc); err != nil { - return nil, nil, err - } - } - isFeature := !uo.Name[0].GetIsExtension() && uo.Name[0].GetNamePart() == featuresFieldName - if isFeature && featuresDefaults == nil { - if interp.lenient { - remain = append(remain, uo) - continue - } - // the "features" option is reserved for use with editions - if err := interp.reporter.HandleErrorf(interp.nodeInfo(node.GetName()).Start(), "%voption 'features' may only be used with editions but file uses %q syntax", mc, interp.syntax()); err != nil { - return nil, nil, err + return nil, err } } mc.Option = uo @@ -944,11 +899,11 @@ func (interp *interpreter) interpretOptions( remain = append(remain, uo) continue } - return nil, nil, err + return nil, err } res.unknown = !isKnownField(optsDesc, res) results = append(results, res) - if isFeature { + if !uo.Name[0].GetIsExtension() && uo.Name[0].GetNamePart() == featuresFieldName { featuresInfo = append(featuresInfo, res) } if optn, ok := node.(*ast.OptionNode); ok { @@ -957,6 +912,10 @@ func (interp *interpreter) interpretOptions( } } + if err := interp.validateFeatures(targetType, msg, featuresInfo); err != nil && !interp.lenient { + return nil, err + } + if interp.lenient { // If we're lenient, then we don't want to clobber the passed in message // and leave it partially populated. So we convert into a copy first @@ -965,121 +924,93 @@ func (interp *interpreter) interpretOptions( // TODO: do this in a more granular way, so we can convert individual // fields and leave bad ones uninterpreted instead of skipping all of // the work we've done so far. - return featuresDefaults, uninterpreted, nil + return uninterpreted, nil } // conversion from dynamic message above worked, so now // it is safe to overwrite the passed in message proto.Reset(opts) proto.Merge(opts, optsClone) - var newFeatures protoreflect.Message - if featuresDefaults != nil { - var err error - newFeatures, err = interp.resolveFeatures(targetType, opts, featuresInfo, featuresDefaults) - if err != nil { - return nil, nil, err - } - } - if interp.container != nil { b, err := interp.toOptionBytes(mc, results) if err != nil { - return nil, nil, err + return nil, err } interp.container.AddOptionBytes(opts, b) } - return newFeatures, remain, nil + return remain, nil } if err := validateRecursive(msg, ""); err != nil { node := interp.file.Node(element) if err := interp.reporter.HandleErrorf(interp.nodeInfo(node).Start(), "error in %s options: %v", descriptorType(element), err); err != nil { - return nil, nil, err + return nil, err } } // now try to convert into the passed in message and fail if not successful if err := cloneInto(opts, msg.Interface(), interp.resolver); err != nil { node := interp.file.Node(element) - return nil, nil, interp.reporter.HandleError(reporter.Error(interp.nodeInfo(node).Start(), err)) - } - - var newFeatures protoreflect.Message - if featuresDefaults != nil { - var err error - newFeatures, err = interp.resolveFeatures(targetType, opts, featuresInfo, featuresDefaults) - if err != nil { - return nil, nil, err - } + return nil, interp.reporter.HandleError(reporter.Error(interp.nodeInfo(node).Start(), err)) } if interp.container != nil { b, err := interp.toOptionBytes(mc, results) if err != nil { - return nil, nil, err + return nil, err } interp.container.AddOptionBytes(opts, b) } - return newFeatures, nil, nil + return nil, nil } -func (interp *interpreter) resolveFeatures(targetType descriptorpb.FieldOptions_OptionTargetType, opts proto.Message, featuresInfo []*interpretedOption, featuresDefault protoreflect.Message) (protoreflect.Message, error) { - optsRef := opts.ProtoReflect() - fld := optsRef.Descriptor().Fields().ByName(featuresFieldName) +func (interp *interpreter) validateFeatures( + targetType descriptorpb.FieldOptions_OptionTargetType, + opts protoreflect.Message, + featuresInfo []*interpretedOption, +) error { + fld := opts.Descriptor().Fields().ByName(featuresFieldName) if fld == nil { // no features to resolve - return featuresDefault, nil + return nil } if fld.IsList() || fld.Message() == nil || fld.Message().FullName() != featureSetName { // features field doesn't look right... abort // TODO: should this return an error? - return featuresDefault, nil + return nil } - features := optsRef.Get(fld).Message() + features := opts.Get(fld).Message() var err error features.Range(func(featureField protoreflect.FieldDescriptor, _ protoreflect.Value) bool { opts, ok := featureField.Options().(*descriptorpb.FieldOptions) - if ok { - var allowed bool - for _, allowedType := range opts.Targets { - if allowedType == targetType { - allowed = true - break - } + if !ok { + return true + } + targetTypes := opts.GetTargets() + var allowed bool + for _, allowedType := range targetTypes { + if allowedType == targetType { + allowed = true + break } - if !allowed { - allowedTypes := make([]string, len(opts.Targets)) - for i, t := range opts.Targets { - allowedTypes[i] = targetTypeString(t) - } - pos := interp.positionOfFeature(featuresInfo, fld.Number(), featureField.Number()) - if len(opts.Targets) == 1 && opts.Targets[0] == descriptorpb.FieldOptions_TARGET_TYPE_UNKNOWN { - // This is how raw_features is defined, to prevent its direct use. - err = interp.reporter.HandleErrorf(pos, "feature field %q may not be used explicitly", featureField.Name()) - } else { - err = interp.reporter.HandleErrorf(pos, "feature %q is allowed on [%s], not on %s", featureField.Name(), strings.Join(allowedTypes, ","), targetTypeString(targetType)) - } + } + if !allowed { + allowedTypes := make([]string, len(targetTypes)) + for i, t := range opts.Targets { + allowedTypes[i] = targetTypeString(t) + } + pos := interp.positionOfFeature(featuresInfo, fld.Number(), featureField.Number()) + if len(opts.Targets) == 1 && opts.Targets[0] == descriptorpb.FieldOptions_TARGET_TYPE_UNKNOWN { + err = interp.reporter.HandleErrorf(pos, "feature field %q may not be used explicitly", featureField.Name()) + } else { + err = interp.reporter.HandleErrorf(pos, "feature %q is allowed on [%s], not on %s", featureField.Name(), strings.Join(allowedTypes, ","), targetTypeString(targetType)) } } return err == nil }) - if err != nil { - return featuresDefault, err - } - resolved, err := resolveFeatures(features, featuresDefault, interp.resolver) - if err != nil { - pos := interp.positionOfFeature(featuresInfo, fld.Number()) - return featuresDefault, interp.reporter.HandleErrorWithPos(pos, err) - } - optsRef.Set(fld, protoreflect.ValueOfMessage(resolved)) - // Set raw features to empty message for the defaults for child elements. - newDefaults := proto.Clone(resolved.Interface()).ProtoReflect() - if rawField := newDefaults.Descriptor().Fields().ByName(rawFeaturesFieldName); rawField != nil { - newDefaults.Set(rawField, newDefaults.NewField(rawField)) - } - return newDefaults, nil + return err } func (interp *interpreter) positionOfFeature(featuresInfo []*interpretedOption, fieldNumbers ...protoreflect.FieldNumber) ast.SourcePos { @@ -1092,8 +1023,7 @@ func (interp *interpreter) positionOfFeature(featuresInfo []*interpretedOption, continue } if len(remainingNumbers) > 0 { - //nolint:gosec // this is not aliasing the loop var; this is taking address of field in slice element - node = findInterpretedFieldForFeature(&info.interpretedField, remainingNumbers) + node = findInterpretedFieldForFeature(&(info.interpretedField), remainingNumbers) } if node != nil { return interp.file.FileNode().NodeInfo(node).Start() @@ -1139,14 +1069,6 @@ func findInterpretedFieldForFeature(opt *interpretedField, path []protoreflect.F return nil } -func (interp *interpreter) syntax() string { - syntax := interp.file.FileDescriptorProto().GetSyntax() - if syntax == "" { - return "proto2" - } - return syntax -} - // isKnownField returns true if the given option is for a known field of the // given options message descriptor and will be serialized using the expected // wire type for that known field. @@ -2332,13 +2254,3 @@ func (interp *interpreter) messageLiteralValue(mc *internal.MessageContext, fiel msgVal: flds, }, nil } - -func editionNode(file file) ast.Node { - if fileNode := file.AST(); fileNode != nil { - if fileNode.Edition == nil { - return fileNode - } - return fileNode.Edition - } - return file.FileNode() -} diff --git a/parser/result.go b/parser/result.go index d1756b33..31b0164a 100644 --- a/parser/result.go +++ b/parser/result.go @@ -30,7 +30,9 @@ import ( "github.com/bufbuild/protocompile/reporter" ) -var supportedEditions = map[string]struct{}{"2023": {}} +var supportedEditions = map[string]descriptorpb.Edition{ + "2023": descriptorpb.Edition_EDITION_2023, +} // NB: protoreflect.Syntax doesn't yet know about editions, so we have to use our own type. type syntaxType int @@ -41,6 +43,19 @@ const ( syntaxEditions ) +func (s syntaxType) String() string { + switch s { + case syntaxProto2: + return "proto2" + case syntaxProto3: + return "proto3" + case syntaxEditions: + return "editions" + default: + return fmt.Sprintf("unknown(%d)", s) + } +} + type result struct { file *ast.FileNode proto *descriptorpb.FileDescriptorProto @@ -125,9 +140,10 @@ func (r *result) createFileDescriptor(filename string, file *ast.FileNode, handl } edition := file.Edition.Edition.AsString() syntax = syntaxEditions + fd.Syntax = proto.String("editions") - fd.Edition = proto.String(edition) - if _, ok := supportedEditions[edition]; !ok { + editionEnum, ok := supportedEditions[edition] + if !ok { nodeInfo := file.NodeInfo(file.Edition.Edition) editionStrs := make([]string, 0, len(supportedEditions)) for supportedEdition := range supportedEditions { @@ -138,6 +154,7 @@ func (r *result) createFileDescriptor(filename string, file *ast.FileNode, handl return } } + fd.Edition = editionEnum.Enum() default: nodeInfo := file.NodeInfo(file) handler.HandleWarningWithPos(nodeInfo.Start(), ErrNoSyntax) diff --git a/parser/validate.go b/parser/validate.go index 1fc01ec2..38adca8f 100644 --- a/parser/validate.go +++ b/parser/validate.go @@ -45,6 +45,10 @@ func validateBasic(res *result, handler *reporter.Handler) { return } + if err := validateNoFeatures(res, syntax, "file options", fd.Options.GetUninterpretedOption(), handler); err != nil { + return + } + _ = walk.DescriptorProtos(fd, func(name protoreflect.FullName, d proto.Message) error { switch d := d.(type) { @@ -53,12 +57,28 @@ func validateBasic(res *result, handler *reporter.Handler) { // exit func is not called when enter returns error return err } + case *descriptorpb.FieldDescriptorProto: + if err := validateField(res, syntax, name, d, handler); err != nil { + return err + } + case *descriptorpb.OneofDescriptorProto: + if err := validateNoFeatures(res, syntax, fmt.Sprintf("oneof %s", name), d.Options.GetUninterpretedOption(), handler); err != nil { + return err + } case *descriptorpb.EnumDescriptorProto: if err := validateEnum(res, syntax, name, d, handler); err != nil { return err } - case *descriptorpb.FieldDescriptorProto: - if err := validateField(res, syntax, name, d, handler); err != nil { + case *descriptorpb.EnumValueDescriptorProto: + if err := validateNoFeatures(res, syntax, fmt.Sprintf("enum value %s", name), d.Options.GetUninterpretedOption(), handler); err != nil { + return err + } + case *descriptorpb.ServiceDescriptorProto: + if err := validateNoFeatures(res, syntax, fmt.Sprintf("service %s", name), d.Options.GetUninterpretedOption(), handler); err != nil { + return err + } + case *descriptorpb.MethodDescriptorProto: + if err := validateNoFeatures(res, syntax, fmt.Sprintf("method %s", name), d.Options.GetUninterpretedOption(), handler); err != nil { return err } } @@ -87,6 +107,23 @@ func validateImports(res *result, handler *reporter.Handler) error { return nil } +func validateNoFeatures(res *result, syntax syntaxType, scope string, opts []*descriptorpb.UninterpretedOption, handler *reporter.Handler) error { + if syntax == syntaxEditions { + // Editions is allowed to use features + return nil + } + if index, err := internal.FindFirstOption(res, handler, scope, opts, "features"); err != nil { + return err + } else if index >= 0 { + optNode := res.OptionNode(opts[index]) + optNameNodeInfo := res.file.NodeInfo(optNode.GetName()) + if err := handler.HandleErrorf(optNameNodeInfo.Start(), "%s: option 'features' may only be used with editions but file uses %s syntax", scope, syntax); err != nil { + return err + } + } + return nil +} + func validateMessage(res *result, syntax syntaxType, name protoreflect.FullName, md *descriptorpb.DescriptorProto, handler *reporter.Handler) error { scope := fmt.Sprintf("message %s", name) @@ -101,30 +138,17 @@ func validateMessage(res *result, syntax syntaxType, name protoreflect.FullName, if index, err := internal.FindOption(res, handler, scope, md.Options.GetUninterpretedOption(), "map_entry"); err != nil { return err } else if index >= 0 { - opt := md.Options.UninterpretedOption[index] - optn := res.OptionNode(opt) - md.Options.UninterpretedOption = internal.RemoveOption(md.Options.UninterpretedOption, index) - valid := false - if opt.IdentifierValue != nil { - if opt.GetIdentifierValue() == "true" { - valid = true - optionNodeInfo := res.file.NodeInfo(optn.GetValue()) - if err := handler.HandleErrorf(optionNodeInfo.Start(), "%s: map_entry option should not be set explicitly; use map type instead", scope); err != nil { - return err - } - } else if opt.GetIdentifierValue() == "false" { - valid = true - md.Options.MapEntry = proto.Bool(false) - } - } - if !valid { - optionNodeInfo := res.file.NodeInfo(optn.GetValue()) - if err := handler.HandleErrorf(optionNodeInfo.Start(), "%s: expecting bool value for map_entry option", scope); err != nil { - return err - } + optNode := res.OptionNode(md.Options.GetUninterpretedOption()[index]) + optNameNodeInfo := res.file.NodeInfo(optNode.GetName()) + if err := handler.HandleErrorf(optNameNodeInfo.Start(), "%s: map_entry option should not be set explicitly; use map type instead", scope); err != nil { + return err } } + if err := validateNoFeatures(res, syntax, scope, md.Options.GetUninterpretedOption(), handler); err != nil { + return err + } + // reserved ranges should not overlap rsvd := make(tagRanges, len(md.ReservedRange)) for i, r := range md.ReservedRange { @@ -144,6 +168,9 @@ func validateMessage(res *result, syntax syntaxType, name protoreflect.FullName, // extensions ranges should not overlap exts := make(tagRanges, len(md.ExtensionRange)) for i, r := range md.ExtensionRange { + if err := validateNoFeatures(res, syntax, scope, r.Options.GetUninterpretedOption(), handler); err != nil { + return err + } n := res.ExtensionRangeNode(r) exts[i] = tagRange{start: r.GetStart(), end: r.GetEnd(), node: n} } @@ -298,6 +325,10 @@ func validateEnum(res *result, syntax syntaxType, name protoreflect.FullName, ed } } + if err := validateNoFeatures(res, syntax, scope, ed.Options.GetUninterpretedOption(), handler); err != nil { + return err + } + allowAlias := false var allowAliasOpt *descriptorpb.UninterpretedOption if index, err := internal.FindOption(res, handler, scope, ed.Options.GetUninterpretedOption(), "allow_alias"); err != nil { @@ -444,8 +475,16 @@ func validateField(res *result, syntax syntaxType, name protoreflect.FullName, f return err } } - } - if syntax == syntaxProto3 { + if index, err := internal.FindOption(res, handler, scope, fld.Options.GetUninterpretedOption(), "packed"); err != nil { + return err + } else if index >= 0 { + optNode := res.OptionNode(fld.Options.GetUninterpretedOption()[index]) + optNameNodeInfo := res.file.NodeInfo(optNode.GetName()) + if err := handler.HandleErrorf(optNameNodeInfo.Start(), "%s: packed option is not allowed in editions; use option features.repeated_field_encoding instead", scope); err != nil { + return err + } + } + } else if syntax == syntaxProto3 { if index, err := internal.FindOption(res, handler, scope, fld.Options.GetUninterpretedOption(), "default"); err != nil { return err } else if index >= 0 { @@ -471,7 +510,7 @@ func validateField(res *result, syntax syntaxType, name protoreflect.FullName, f } } - return nil + return validateNoFeatures(res, syntax, scope, fld.Options.GetUninterpretedOption(), handler) } type tagRange struct { diff --git a/parser/validate_test.go b/parser/validate_test.go index a3ef4138..7f523f1f 100644 --- a/parser/validate_test.go +++ b/parser/validate_test.go @@ -147,12 +147,12 @@ func TestBasicValidation(t *testing.T) { expectedErr: `test.proto:1:38: extend sections must define at least one extension`, }, "failure_explicit_map_entry_option": { - contents: `message Foo { option map_entry = true; } message Bar { optional Foo foo = 1; }`, - expectedErr: `test.proto:1:34: message Foo: map_entry option should not be set explicitly; use map type instead`, + contents: `message Foo { option map_entry = true; }`, + expectedErr: `test.proto:1:22: message Foo: map_entry option should not be set explicitly; use map type instead`, }, - "success_explicit_map_entry_option": { - // okay if explicit setting is false - contents: `message Foo { option map_entry = false; }`, + "failure_explicit_map_entry_option_false": { + contents: `message Foo { option map_entry = false; }`, + expectedErr: `test.proto:1:22: message Foo: map_entry option should not be set explicitly; use map type instead`, }, "failure_proto2_requires_label": { contents: `syntax = "proto2"; message Foo { string s = 1; }`, @@ -272,9 +272,8 @@ func TestBasicValidation(t *testing.T) { contents: `message Foo { reserved 1, 2 to 10, 11 to 20; extensions 21 to 22; }`, }, "failure_message_reserved_start_after_end": { - contents: `message Foo { reserved 10 to 1; }`, - expectedErr: `test.proto:1:24: range, 10 to 1, is invalid: start must be <= end`, - expectedDiffWithProtoc: true, // bug in protoc: https://github.com/protocolbuffers/protobuf/issues/13442 + contents: `message Foo { reserved 10 to 1; }`, + expectedErr: `test.proto:1:24: range, 10 to 1, is invalid: start must be <= end`, }, "failure_message_extensions_start_after_end": { contents: `message Foo { extensions 10 to 1; }`, @@ -692,8 +691,7 @@ func TestBasicValidation(t *testing.T) { message Foo { reserved "foo", "_bar123", "A_B_C_1_2_3"; }`, - expectedErr: `test.proto:3:55: must use identifiers, not string literals, to reserved names with editions`, - expectedDiffWithProtoc: true, // protoc v24.0-rc2 doesn't yet include support for identifiers + expectedErr: `test.proto:3:55: must use identifiers, not string literals, to reserved names with editions`, }, "failure_enum_invalid_reserved_name": { contents: `syntax = "proto3"; @@ -751,8 +749,7 @@ func TestBasicValidation(t *testing.T) { BAR = 0; reserved "foo", "_bar123", "A_B_C_1_2_3"; }`, - expectedErr: `test.proto:4:55: must use identifiers, not string literals, to reserved names with editions`, - expectedDiffWithProtoc: true, // protoc v24.0-rc2 doesn't yet include support for identifiers + expectedErr: `test.proto:4:55: must use identifiers, not string literals, to reserved names with editions`, }, "failure_message_reserved_ident_proto2": { contents: `syntax = "proto2"; @@ -773,7 +770,6 @@ func TestBasicValidation(t *testing.T) { message Foo { reserved foo, _bar123, A_B_C_1_2_3; }`, - expectedDiffWithProtoc: true, // protoc v24.0-rc2 doesn't yet include support for identifiers }, "failure_enum_reserved_ident_proto2": { contents: `syntax = "proto2"; @@ -797,7 +793,89 @@ func TestBasicValidation(t *testing.T) { BAR = 0; reserved foo, _bar123, A_B_C_1_2_3; }`, - expectedDiffWithProtoc: true, // protoc v24.0-rc2 doesn't yet have support for identifiers with editions + }, + "failure_use_of_packed_with_editions": { + contents: `edition = "2023"; + message Foo { + repeated bool foo = 1 [packed=false]; + }`, + expectedErr: `test.proto:3:69: field Foo.foo: packed option is not allowed in editions; use option features.repeated_field_encoding instead`, + }, + "failure_use_of_features_without_editions_file": { + contents: `syntax = "proto3"; + option features.utf8_validation = VERIFY; + message Foo { + string foo = 1; + }`, + expectedErr: `test.proto:2:51: file options: option 'features' may only be used with editions but file uses proto3 syntax`, + }, + "failure_use_of_features_without_editions_message": { + contents: `syntax = "proto3"; + message Foo { + option features = {}; + string foo = 1; + }`, + expectedErr: `test.proto:3:53: message Foo: option 'features' may only be used with editions but file uses proto3 syntax`, + }, + "failure_use_of_features_without_editions_field": { + contents: `syntax = "proto3"; + message Foo { + string foo = 1 [features.field_presence = LEGACY_REQUIRED]; + }`, + expectedErr: `test.proto:3:62: field Foo.foo: option 'features' may only be used with editions but file uses proto3 syntax`, + }, + "failure_use_of_features_without_editions_oneof": { + contents: `syntax = "proto3"; + message Foo { + oneof x { + option features = {}; + string foo = 1; + } + }`, + expectedErr: `test.proto:4:55: oneof Foo.x: option 'features' may only be used with editions but file uses proto3 syntax`, + }, + "failure_use_of_features_without_editions_ext_range": { + contents: `syntax = "proto2"; + message Foo { + extensions 1 to 100 [features={}]; + }`, + expectedErr: `test.proto:3:67: message Foo: option 'features' may only be used with editions but file uses proto2 syntax`, + }, + "failure_use_of_features_without_editions_enum": { + contents: `syntax = "proto2"; + enum Foo { + option features.enum_type = CLOSED; + VALUE = 0; + }`, + expectedErr: `test.proto:3:53: enum Foo: option 'features' may only be used with editions but file uses proto2 syntax`, + }, + "failure_use_of_features_without_editions_enum_val": { + contents: `syntax = "proto2"; + enum Foo { + VALUE = 0 [features={}]; + }`, + expectedErr: `test.proto:3:57: enum value VALUE: option 'features' may only be used with editions but file uses proto2 syntax`, + }, + "failure_use_of_features_without_editions_service": { + contents: `syntax = "proto3"; + message Foo {} + service FooService { + option features = {}; + rpc Do(Foo) returns (foo); + } + `, + expectedErr: `test.proto:4:53: service FooService: option 'features' may only be used with editions but file uses proto3 syntax`, + }, + "failure_use_of_features_without_editions_method": { + contents: `syntax = "proto3"; + message Foo {} + service FooService { + rpc Do(Foo) returns (foo) { + option features = {}; + } + } + `, + expectedErr: `test.proto:5:55: method FooService.Do: option 'features' may only be used with editions but file uses proto3 syntax`, }, } diff --git a/sourceinfo/source_code_info.go b/sourceinfo/source_code_info.go index b5415bfc..908fb170 100644 --- a/sourceinfo/source_code_info.go +++ b/sourceinfo/source_code_info.go @@ -139,6 +139,9 @@ func generateSourceInfoForFile(opts OptionIndex, sci *sourceCodeInfo, file *ast. if file.Syntax != nil { sci.newLocWithComments(file.Syntax, append(path, internal.FileSyntaxTag)) } + if file.Edition != nil { + sci.newLocWithComments(file.Edition, append(path, internal.FileEditionTag)) + } var depIndex, pubDepIndex, weakDepIndex, optIndex, msgIndex, enumIndex, extendIndex, svcIndex int32