Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve handling of the generic baggage fields #5656

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

vstakhov
Copy link
Contributor

@vstakhov vstakhov commented Jun 9, 2022

This PR addresses issue paritytech/orchestra#6 and allows orchestra proc-macros to deal with the generics in baggage fields.

  • Support of a single or multiple Gen<T>, Gen<Gen<T>>
  • Check and test complex containers HashMap<Gen<T>, Gen<U>>
  • Think about lifetimes - not relevant in this case, as subsystems must be static
  • Think about const generic parameters - not needed (yet), support can be added if any need arises

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 9, 2022
@vstakhov vstakhov added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jun 10, 2022
@vstakhov vstakhov marked this pull request as ready for review June 10, 2022 09:33
@vstakhov vstakhov added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jun 10, 2022
@vstakhov vstakhov requested review from drahnr and sandreim June 10, 2022 09:34
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good! Thank you

.iter()
.filter_map(|gen| match gen {
syn::GenericArgument::Type(ty) =>
try_type_to_path(ty.clone(), span.clone()).ok(),
Copy link
Contributor

@drahnr drahnr Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This swallows somerrors, which we shouldn't swallow but expose as part of the return type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GenericArgument enum looks the following:

pub enum GenericArgument {
        /// A lifetime argument.
        Lifetime(Lifetime),
        /// A type argument.
        Type(Type),
        /// A binding (equality constraint) on an associated type: the `Item =
        /// u8` in `Iterator<Item = u8>`.
        Binding(Binding),
        /// An associated type bound: `Iterator<Item: Display>`.
        Constraint(Constraint),
        /// A const expression. Must be inside of a block.
        ///
        /// NOTE: Identity expressions are represented as Type arguments, as
        /// they are indistinguishable syntactically.
        Const(Expr),
    }

Lifetimes are irrelevant - we can emit error on that, but it will be also emitted during compilation, as lifetimes are not efficiently supported. Binding is more interesting here, as there could be something like Item = T where T is indeed a generic. Constraint can also have generic inside. Probably, it is not a bad option just to throw an error there as it will be a compile error afterwards.

@vstakhov
Copy link
Contributor Author

Well, adding strict checks has revealed that we have no real support of anything but paths, for example tuples. It is bad and requires some more changes...

@vstakhov
Copy link
Contributor Author

To clarify, the existing implementation supports Vec<(type1, type2)> but not (type1, type2).

@vstakhov
Copy link
Contributor Author

With this approach, baggage type is now Type not Path that allows to put more syntax elements into baggage, such as tuples, functors and so on. Not all types could be flattened so far, but this could be extended if needed. Apparently, with Path restriction it was impossible to use such types in the builder's setters as well.

@eskimor
Copy link
Member

eskimor commented Jun 14, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit d2fb2d7 into master Jun 14, 2022
@paritytech-processbot paritytech-processbot bot deleted the vstakhov-issue-5654 branch June 14, 2022 12:38
al3mart pushed a commit that referenced this pull request Jul 14, 2022
* Parse generic baggage types more carefully to preserve inner structure

* Add example

* Way too many clones

* Allow multiple generic arguments for baggage fields

* Try to detect errors earlier

* Support more types for the baggage fields, get rid of the path constraint
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants