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

Interpolate non-repeating variable inside iterating pattern #4

Closed
Goncalerta opened this issue Jan 30, 2019 · 6 comments
Closed

Interpolate non-repeating variable inside iterating pattern #4

Goncalerta opened this issue Jan 30, 2019 · 6 comments
Labels
enhancement New feature or request

Comments

@Goncalerta
Copy link
Owner

Goncalerta commented Jan 30, 2019

This issue tracks the follow up of dtolnay/quote#7.

My concern about this feature is that it would make interpolation of types that implement both ToTokens and Iterator<Item=ToTokens> ambiguous. (example: TokenStream, a type that will probably come up alot in interpolations).

In fact, I think the most common use for TokenStreams would be to be used as a ToTokens rather than an iterator.

As a rough made up example:

let path = quote!{ ::some::path::to::an::object };
let construct_fn = Ident::new("new", Span::call_site());
let i = 0..5;
let output = quote!{
    #(object[#i] = #path::#construct_fn();)*
}

As TokenStream implements Iterator<Item=TokenTree>, this would (understandably) give a result that wasn't expected from the user. I fear this could become confusing in much more complex macros.

By not allowing non-repeating variables altogether, users would be forced to explicitly turn every item they want to use as ToTokens before passing them to the quote

let path = quote!{ ::some::path::to::an::object };
let construct_fn = Ident::new("new", Span::call_site());
let i = 0..5;

let path = std::iter::repeat_with(|| &path);
let construct_fn = std::iter::repeat_with(|| &construct_fn);
let output = quote!{
    #(object[#i] = #path::#construct_fn();)*
}

This case is not ambiguous and less error prone, as everything inside #(...)* must be an iterator and the variables that would intended to be used as ToTokens are explicitly stated as so with the std::iter::repeat_with before the quote!.

@Goncalerta Goncalerta added the question Further information is requested label Jan 30, 2019
@dtolnay
Copy link

dtolnay commented Jan 30, 2019

Two alternatives:

  • How about a blacklist of types that would not get treated as repeating? This list would include TokenStream.

  • How about using a different set of traits than ToTokens and IntoIterator inside a repetition? You could define proc_quote::Repeat which is implemented for TokenStream, Vec, &[T], and T where T: Iterator that defines the behavior when interpolated in a repetition.

@Goncalerta Goncalerta added the enhancement New feature or request label Feb 20, 2019
@Goncalerta
Copy link
Owner Author

After a lot of thought and many dead ends I think I'm finally getting on to something. The current progress to solve this issue is in the repeat branch.

The main change here is the introduction of the Repeat trait, which defines which types
are allowed inside a repeating pattern (#(...)*).

What does Repeat:

  • Iterators, by iterating until next returns None.
  • Slices (includes Vec, arrays, anything that implements Borrow<[T]>) by iterating through every element (as a reference), but without consuming the original data.
  • ToTokens, by repeating the same token infinitely.

What does NOT Repeat:

  • IntoIterator, to avoid ambiguity (Ex. Which behavior would have been used for Vec, which implements both IntoIterator and Borrow<[T]>? Which behavior would have been used for TokenStream, which implements both IntoIterator and ToTokens?). To use the iterator, call into_iter explicitely.
  • Ambiguous types (should only happen very rarely, or really never at all) that implement at least two of the Repeat traits. In the very unlikely case this happens, disambiguate by wrapping the type under some structure that only implements the trait you desire to use.

Problem with the current implementation

However, I'm having trouble fixing name collision. something.as_repeat() will collide with other functions named as_repeat, either by another trait in scope or the impl of the type. I know there is the fully qualified syntax Repeat::as_repeat(something), however, that would raise another problem.

While iterators have to be consumed to be used (transfer ownership), slices and ToTokens shouldn't (borrow reference). On the other hand, the macro doesn't have access to the type of the variable, to know if it should borrow or consume, so it must generate the same code to work in both cases. This means Repeat::as_repeat(&something) couldn't be generated, otherwise iterators wouldn't work. Still, by using Repeat::as_repeat(something), the compiler doesn't infer the reference for some reason (while something.as_repeat() does), so slices and ToTokens wouldn't work directly.

Two non-ideal solutions

  1. The first possibility would be to keep this unfortunate collision, which would be messy, but in practice wouldn't actually be a big issue if a name that no one would use was chosen (something like __proc_macro__as_repeat()).

  2. The second possibility would be to use the fully qualified syntax, but the user would need to borrow the variables before passing them into the macro, which could get annoying:

let a = vec!["a", "b", "c"];
let b = [5,6,7];
let c = 5;
let d = "a";
let e = X;

let a = &a;
let b = &b;
let c = &c;
let d = &d;
let e = &e;
let q = quote!{ #(#a #b #c #d #e)* };

I'm more inclined to the first solution because it would have less practical impact on the user. But I would really like to find a better way to fix this.

@Goncalerta
Copy link
Owner Author

@dtolnay you have much experience with macros, so I'd like to know if some trick comes to your mind that would allow to fix this problem, such as a way to call something.as_repeat() without collision, or infer the reference in a function parameter so Repeat::as_repeat(something) could be used.

@Goncalerta
Copy link
Owner Author

Opened #7 for the name collision issue.

@Goncalerta Goncalerta removed the question Further information is requested label Feb 25, 2019
@dtolnay
Copy link

dtolnay commented Feb 25, 2019

Nothing comes to mind to solve either the name collision or the inferring when a variable needs to be taken as a reference. But I think your method name is unique enough to not worry about collisions.

@eddyb
Copy link

eddyb commented Mar 21, 2019

There is a problem here, AFAICT something like this will repeat ad infinitum without warning (and end up allocating GBs of RAM until stopped by the OOM killer):

let x = quote!(_);
quote(#(#x)*);

If iter::repeat is involved, then its implementation of size_hint suggests this could be detected at runtime by asserting that combined_iterator.size_hint().0 < usize::MAX (but I'm not familiar enough with the implementation to be sure).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants