Remove bounds on Config trait that aren't strictly necessary #389
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…move unnecessary bounds on Config
dvdplm
approved these changes
Jan 13, 2022
Comment on lines
+35
to
+37
| // Note: the 'static bound isn't strictly required, but currently deriving TypeInfo | ||
| // automatically applies a 'static bound to all generic types (including this one), | ||
| // and so until that is resolved, we'll keep the (easy to satisfy) constraint here. |
There was a problem hiding this comment.
That isn't very likely to ever occur I think?
0623forbidden
pushed a commit
to DEIPworld/substrate-subxt
that referenced
this pull request
Feb 15, 2022
…ech#389) * Use Derivative to skip bounds on T when they aren't necessary, and remove unnecessary bounds on Config * loosen a couple more derive bounds * Use PhantomDataSendSync to avoid accidentally removing Send+Sync bounds
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We use types implementing the
Configtrait in Subxt, but only at compile-time; we never instantiate an instance of them. And yet, the Config trait has various bounds on it (Debug,Clone,Copy,Send,Syncetc).This PR removes those bounds (well, mostly;
'staticis still there for now and should be able to go away eventually, but that should never complicate anything in real use).The reason I think that these bounds existed in the first place is that Rust's inbuilt "derive" mechanism is quite primitive (eg deriving
Clonewill derive something likeimpl <T: Clone> Clone for MyType<T>; theTalso needs to beClone). As a result, we ended up with a bunch of additional bounds on theConfigtype so that it could be passed around as a generic typeTand would still satisfy these derives.Removing the bounds on Config has pros and cons:
Pros:
Configis never instantiated, and so it shouldn't need any of these bounds.Configon their own types; they don't need to pointlessly derive anything.Cons:
derivativecrate to allow ignoring theTwhen deriving various traits on things that useConfig. (we could manually derive the traits everywhere, but that would be needlessly verbose). This additional complexity will apply to anything else that takes aT: Configtype.PhantomDataSendSync<T>type (which is justPhantomData + Send + Sync) to ensure that the lack ofSend+SynconConfigdoesn't translate to a lack ofSend+Syncon types using the config.I think I lean towards being more "correct" at the cost of the additional complexity (and from a user facing POV, they won't see any of the added complexity, but will benefit a little from
Configbeing easier to implement.That said, I'm curious to hear people's opinions on the matter!