Skip to content

feature: Add add_interpolation assist #12020

New issue

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

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

Already on GitHub? # to your account

Closed
wants to merge 3 commits into from

Conversation

dotjpg3141
Copy link

@dotjpg3141 dotjpg3141 commented Apr 17, 2022

This adds an assist, which adds interpolations in string literals.

@bors
Copy link
Contributor

bors commented May 1, 2022

☔ The latest upstream changes (presumably #12118) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

I don't like saying this since I'm sure a lot of work has went into this but we already have this in form of postfix completions and I don't think I see value of having an assist here.

Code_IRTqGxwAFh

@flodiebold
Copy link
Member

I think this can still be pretty useful since it also allows you to add interpolations to existing macro calls. (I basically never think to use the postfix completion, but this one I might remember to use.)

@Veykril
Copy link
Member

Veykril commented May 17, 2022

Alright, though I am sure there is some potential for code re-use here.

@Veykril Veykril self-assigned this Jun 21, 2022
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Sorry this took so long to get back to, I had a bunch of things on my plate the past weeks.

This seems mostly okay, its a bit too far on the AST-only front, but that's not really something we can change right now. We should also look into creating some general format-ish "parsing" tools as this is something we can probably re-use in other places.

With that said I think we can merge this with the debug_assert removed. Thanks!

Comment on lines +106 to +144
fn match_format_macro_arg_delims(string_ast: &ast::String) -> Option<Vec<SyntaxToken>> {
let string_token = string_ast.syntax();

let macro_call = ast::MacroCall::cast(string_token.ancestors().nth(1)?)?;
let macro_name = macro_call.path()?.segment()?.name_ref()?;

let format_arg_idx = match macro_name.text().as_str() {
"eprint" | "eprintln" | "format" | "panic" | "print" | "println" | "todo"
| "unimplemented" | "unreachable" => 0,
"assert" | "debug_assert" | "write" | "writeln" => 1,
"assert_eq" | "assert_ne" | "debug_assert_eq" | "debug_assert_ne" => 2,
_ => return None,
};

let token_tree = macro_call.token_tree()?;
let left_delim = token_tree.left_delimiter_token()?;
let right_delim = token_tree.right_delimiter_token()?;
let mut delimiters: Vec<SyntaxToken> = token_tree
.token_trees_and_tokens()
.flat_map(|not| not.as_token().cloned())
.filter(|token| token.kind() == T![,] || token == &left_delim || token == &right_delim)
.collect();

let format_arg_start = delimiters.get(format_arg_idx)?.text_range().end();
let format_arg_end = delimiters.get(format_arg_idx + 1)?.text_range().start();
let format_arg_range = TextRange::new(format_arg_start, format_arg_end);

if !format_arg_range.contains_range(string_token.text_range()) {
return None;
}

delimiters.drain(..format_arg_idx + 1);

if let Some(first_named) = first_named_argument(token_tree) {
delimiters.retain(|delim| delim.text_range().end() <= first_named.text_range().start());
}

Some(delimiters)
}
Copy link
Member

@Veykril Veykril Jul 20, 2022

Choose a reason for hiding this comment

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

Here we could be a bit smarter by using

pub fn is_format_string(string: &ast::String) -> bool {

For that to work we need to descend the string token into macros first though, that can be done by doing

let descended_string = sema.descend_into_macros_single(string_token);

and then passing the descended_string to is_format_string. That would allow us to not do this purely textually, but that doesn't fetch us the arg index so might be a bit more involved, so I think the current approach here is fine as is for now. Using is_format_string would also allow more than just the std macros, which doesn't really work in the general case since we don't know where the extra args should go anyways.

name_state = NameState::Equal;
}
(T![=], NameState::Equal) => {
debug_assert!(name_candiate.is_some());
Copy link
Member

Choose a reason for hiding this comment

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

This debug_assert seems fishy, what if we have something like format!("{foo}", =), not really a valid call but still something that could be written. So this assert should go I think

@Veykril Veykril removed their assignment Aug 31, 2022
@Veykril Veykril marked this pull request as draft September 27, 2022 13:18
@Veykril
Copy link
Member

Veykril commented Nov 2, 2022

Closing this for now, if you find the time to come back to this feel free to re-open the issue.

@Veykril Veykril closed this Nov 2, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants