Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix field/method conflict #50

Open
Tracked by #63
fdymylja opened this issue Dec 10, 2021 · 4 comments · Fixed by #51
Open
Tracked by #63

fix field/method conflict #50

fdymylja opened this issue Dec 10, 2021 · 4 comments · Fixed by #51
Milestone

Comments

@fdymylja
Copy link
Contributor

fdymylja commented Dec 10, 2021

Given the following proto message type:

message Foo {
  string type = 1;
}

We will produce a Foo.Type field and a Foo.Type() protoreflect.MessageType method, this causes code conflicts.

If a message uses a reserved name, for the sake of simplicity, we should suffix the field with an underscore _.

I would also output a warning message that this is bad, and we might break it in the future (reason for breaking is that we are incompatible with protoc field codegen which I do not think is good).

@aaronc
Copy link
Member

aaronc commented Dec 15, 2021

Getting these warnings now:

2021/12/15 11:12:07 Message cosmos.base.abci.v1beta1.StringEvent contains the reserved field name cosmos.base.abci.v1beta1.StringEvent.type which conflicts with protoreflect.Message interface implementation.
This field will be suffixed with an underscore '_'.
If you can change the message field name, please do so.
In a future iteration of pulsar we may make a breaking change to this practice in order to be compliant with field naming of the original golang protobuf implementation.
2021/12/15 11:12:07 Message tendermint.crypto.ProofOp contains the reserved field name tendermint.crypto.ProofOp.type which conflicts with protoreflect.Message interface implementation.
This field will be suffixed with an underscore '_'.
If you can change the message field name, please do so.
In a future iteration of pulsar we may make a breaking change to this practice in order to be compliant with field naming of the original golang protobuf implementation.
2021/12/15 11:12:07 Message tendermint.abci.RequestCheckTx contains the reserved field name tendermint.abci.RequestCheckTx.type which conflicts with protoreflect.Message interface implementation.
This field will be suffixed with an underscore '_'.
If you can change the message field name, please do so.
In a future iteration of pulsar we may make a breaking change to this practice in order to be compliant with field naming of the original golang protobuf implementation.
2021/12/15 11:12:07 Message tendermint.abci.Event contains the reserved field name tendermint.abci.Event.type which conflicts with protoreflect.Message interface implementation.
This field will be suffixed with an underscore '_'.
If you can change the message field name, please do so.
In a future iteration of pulsar we may make a breaking change to this practice in order to be compliant with field naming of the original golang protobuf implementation.
2021/12/15 11:12:07 Message tendermint.abci.Evidence contains the reserved field name tendermint.abci.Evidence.type which conflicts with protoreflect.Message interface implementation.
This field will be suffixed with an underscore '_'.
If you can change the message field name, please do so.
In a future iteration of pulsar we may make a breaking change to this practice in order to be compliant with field naming of the original golang protobuf implementation.
2021/12/15 11:12:07 Message tendermint.types.Vote contains the reserved field name tendermint.types.Vote.type which conflicts with protoreflect.Message interface implementation.
This field will be suffixed with an underscore '_'.
If you can change the message field name, please do so.
In a future iteration of pulsar we may make a breaking change to this practice in order to be compliant with field naming of the original golang protobuf implementation.
2021/12/15 11:12:07 Message tendermint.types.Proposal contains the reserved field name tendermint.types.Proposal.type which conflicts with protoreflect.Message interface implementation.
This field will be suffixed with an underscore '_'.
If you can change the message field name, please do so.
In a future iteration of pulsar we may make a breaking change to this practice in order to be compliant with field naming of the original golang protobuf implementation.

7 instances. Is this enough to merit finding another work-around?

@aaronc
Copy link
Member

aaronc commented Dec 15, 2021

Plus a bunch of errors:

cosmos/tx/signing/v1beta1/signing.pulsar.go:1154:42: v.ProtoReflect undefined (type *SignatureDescriptor_Data_Single has no field or method ProtoReflect)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1160:42: v.ProtoReflect undefined (type *SignatureDescriptor_Data_Multi has no field or method ProtoReflect)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1235:78: (*SignatureDescriptor_Data_Single)(nil).ProtoReflect undefined (type *SignatureDescriptor_Data_Single has no field or method ProtoReflect)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1237:47: v.Single.ProtoReflect undefined (type *SignatureDescriptor_Data_Single has no field or method ProtoReflect)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1239:78: (*SignatureDescriptor_Data_Single)(nil).ProtoReflect undefined (type *SignatureDescriptor_Data_Single has no field or method ProtoReflect)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1243:77: (*SignatureDescriptor_Data_Multi)(nil).ProtoReflect undefined (type *SignatureDescriptor_Data_Multi has no field or method ProtoReflect)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1245:46: v.Multi.ProtoReflect undefined (type *SignatureDescriptor_Data_Multi has no field or method ProtoReflect)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1247:77: (*SignatureDescriptor_Data_Multi)(nil).ProtoReflect undefined (type *SignatureDescriptor_Data_Multi has no field or method ProtoReflect)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1270:36: impossible type assertion:
        *SignatureDescriptor_Data_Single does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1273:36: impossible type assertion:
        *SignatureDescriptor_Data_Multi does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
cosmos/tx/signing/v1beta1/signing.pulsar.go:1273:36: too many errors

@aaronc
Copy link
Member

aaronc commented Jan 6, 2022

Let's reopen this. I think it would be good to do the painful thing and not have to have the warning about fields getting renamed

@aaronc aaronc reopened this Jan 6, 2022
@aaronc aaronc added this to the v1.0.0 milestone Jan 6, 2022
@aaronc aaronc mentioned this issue Feb 25, 2022
3 tasks
@aaronc
Copy link
Member

aaronc commented Mar 29, 2022

If there is a type with a field method conflict, we should do codegen using a struct for the fast refletion wrapper instead of an alias to avoid the field method conflict.

Currently we're generating a fast reflection wrapper like this

type fastReflection_A A

This causes a field method conflict because Type would be a struct field and an interface method on the wrapper. Using a struct instead of an alias for the wrapper would avoid this. Ex:

type fastReflection_A struct {
  a A
}

We should only use a struct when there is a conflict, because an alias is more performant otherwise.

elias-orijtech added a commit to elias-orijtech/cosmos-proto that referenced this issue Mar 24, 2023
Solve the clashes by introducing a level of indirection, replacing

type fastReflection_A A

with

type fastReflection_A struct {
 x *A
}

Updates cosmos#50
elias-orijtech added a commit to elias-orijtech/cosmos-proto that referenced this issue Mar 24, 2023
Now that fast reflection has struct wrappers, field/method name clashes
are no longer possible.

Fixes cosmos#50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Backlog
Development

Successfully merging a pull request may close this issue.

3 participants