fix: split float literal tokens at `.` to fix parsing of tuple field accesses #12149

jonas-schievink · 2022-05-04T15:11:59Z

This introduces an ast::FloatLiteral node, changes the FLOAT_LITERAL token to FLOAT_LITERAL_PART, and splits any float literal at the . character, into a FLOAT_LITERAL_PART, and optional DOT and trailing FLOAT_LITERAL_PART token. The tokens are reassembled when passing them to a macro as a tt::Literal.

A slight regression is introduced in how float literals are highlighted: the . is now highlighted as an operator. I've tried to fix this but couldn't figure out how to highlight the whole ast::FloatLiteral node as a unit. This is fixed

Fixes #1109
Fixes #10492
Fixes #12107
Fixes #10560
Fixes #11487

Veykril · 2022-05-04T16:22:36Z

A slight regression is introduced in how float literals are highlighted: the . is now highlighted as an operator. I've tried to fix this but couldn't figure out how to highlight the whole ast::FloatLiteral node as a unit.

You most likely can't do that easily. So the best approach here would be to check when highlighting the T![.] whether it has a float literal as its parent and then change its highlighting accordingly.
(on that note, now that float literals are nodes anyways, I wonder if we want to split float/int suffixes off into their own tokens, there was an issue asking for specific highlighting for those, and that would make it easier)

jonas-schievink · 2022-05-04T16:47:41Z

Ah, it looks like the problem was that token trees don't use an ast::FloatLiteral parent for them, so it doesn't work in macro invocations. Maybe they should also wrap them in a node, that might also make correct conversion to tt::Literal easier.

Veykril · 2022-05-04T17:23:21Z

Ah that is a problem indeed (as long as they aren't being downmapped that is), wrapping them inside of token trees might be better in that case. (Though that would unfortunately mean token trees no longer only contain other token tree syntax nodes which I think some code might be assuming right now)

jonas-schievink · 2022-05-04T17:27:59Z

Hackier than I'd like, but it seems to work. Oh well, not like we can escape hacks for this.

bors · 2022-05-05T08:10:07Z

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

Veykril · 2022-05-05T08:13:56Z

Hm, ye some code actually does make the assumption about only TokenTree nodes being in TokenTrees... Though this might not be relevant, iirc this is mainly used for derives, or it might not even be used at all anymore

rust-analyzer/crates/syntax/src/ast/node_ext.rs

Lines 739 to 746 in e789d73

    
           pub fn token_trees_and_tokens( 
        
               &self, 
        
           ) -> impl Iterator<Item = NodeOrToken<ast::TokenTree, SyntaxToken>> { 
        
               self.syntax().children_with_tokens().filter_map(|not| match not { 
        
                   NodeOrToken::Node(node) => ast::TokenTree::cast(node).map(NodeOrToken::Node), 
        
                   NodeOrToken::Token(t) => Some(NodeOrToken::Token(t)), 
        
               }) 
        
           }

jonas-schievink · 2022-05-05T13:44:19Z

This is more difficult to fix than I thought. With this PR we fail to parse 0e0f32.sin(); correctly because the . is treated as part of the float literal during parsing. I guess that means we need more token types to handle the possible cases:

Single token: 0e0f32
Two tokens, last one's a DOT: 1.
3 tokens, middle one's a DOT: 1.1

Urgh

jonas-schievink · 2022-05-05T14:41:56Z

New approach: The first token of a float literal is now one of FLOAT_NUMBER_START_{0,1,2}, indicating the number of following tokens that are part of the literal.

I've removed the FloatLiteral node in token trees, it's now unnecessary since these new tokens let us tell exactly how many tokens are part of the literal. Highlighting . can just check if the preceding token is FLOAT_NUMBER_START_1 or FLOAT_NUMBER_START_2 to know that it is part of a float number.

Veykril · 2022-05-05T14:59:51Z

Sounds good to me (that is as good as a hack can sound).

jonas-schievink · 2022-05-05T15:35:05Z

@bors r+

bors · 2022-05-05T15:35:06Z

📌 Commit d974a0b has been approved by jonas-schievink

bors · 2022-05-05T15:35:12Z

⌛ Testing commit d974a0b with merge cc9ae2b...

bors · 2022-05-05T15:45:01Z

☀️ Test successful - checks-actions
Approved by: jonas-schievink
Pushing cc9ae2b to master...

cynecx · 2022-05-05T23:49:43Z

I think this broke macro-expansion (reduced snippet from num-traits):

macro_rules! constant {
    ($( $method:ident () -> $ret:expr ; )*)
        => {$(
            #[inline]
            fn $method() -> Self {
                $ret
            }
        )*};
}

trait M {
    fn neg_zero() -> Self;
}

impl M for f32 {
    constant! {
        neg_zero() -> -0.0;
    }
}

I am also getting this error with an unrelated project. (I can reproduce this with an empty crate with a single dependency: num-traits):

Output

version: 0216ca61a 2022-05-05 dev
request: textDocument/semanticTokens/range SemanticTokensRangeParams {
    work_done_progress_params: WorkDoneProgressParams {
        work_done_token: None,
    },
    partial_result_params: PartialResultParams {
        partial_result_token: None,
    },
    text_document: TextDocumentIdentifier {
        uri: Url {
            scheme: "file",
            cannot_be_a_base: false,
            username: "",
            password: None,
            host: None,
            port: None,
            query: None,
            fragment: None,
        },
    },
    range: Range {
        start: Position {
            line: 0,
            character: 0,
        },
        end: Position {
            line: 12,
            character: 0,
        },
    },
}

> collect_items MacroCall: constant!{nan()->f32::NAN;infinity()->f32::INFINITY;neg_infinity()->f32::NEG_INFINITY;neg_zero()-> -0.0;min_value()->f32::MIN;min_positive_value()->f32::MIN_POSITIVE;epsilon()->f32::EPSILON;max_value()->f32::MAX;}#[

thread '<unnamed>' panicked at 'Punct {
    char: 'f',
    spacing: Joint,
    id: TokenId(
        74,
    ),
} is not a valid punct', crates/mbe/src/to_parser_input.rs:61:48
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: mbe::to_parser_input::to_parser_input
   3: mbe::tt_iter::TtIter::expect_fragment
   4: mbe::expander::matcher::match_loop
   5: mbe::expander::expand_rules
   6: mbe::DeclarativeMacro::expand
   7: hir_expand::db::TokenExpander::expand
   8: <hir_expand::db::MacroExpandQuery as salsa::plumbing::QueryFunction>::execute
   9: salsa::runtime::Runtime::execute_query_implementation
  10: salsa::derived::slot::Slot<Q,MP>::read_upgrade
  11: salsa::derived::slot::Slot<Q,MP>::read
  12: <salsa::derived::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::try_fetch
  13: salsa::QueryTable<Q>::get
  14: <DB as hir_expand::db::AstDatabase>::macro_expand
  15: salsa::runtime::Runtime::execute_query_implementation
  16: salsa::derived::slot::Slot<Q,MP>::read_upgrade
  17: salsa::derived::slot::Slot<Q,MP>::read
  18: <salsa::derived::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::try_fetch
  19: salsa::QueryTable<Q>::get
  20: <DB as hir_expand::db::AstDatabase>::macro_expand_error
  21: hir_def::body::Expander::enter_expand_inner
  22: hir_def::body::Expander::enter_expand
  23: hir_def::data::AssocItemCollector::collect
  24: hir_def::data::ImplData::impl_data_query
  25: salsa::runtime::Runtime::execute_query_implementation
  26: salsa::derived::slot::Slot<Q,MP>::read_upgrade
  27: salsa::derived::slot::Slot<Q,MP>::read
  28: <salsa::derived::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::try_fetch
  29: <DB as hir_def::db::DefDatabase>::impl_data::__shim
  30: hir_ty::lower::impl_trait_query
  31: salsa::runtime::Runtime::execute_query_implementation
  32: salsa::derived::slot::Slot<Q,MP>::read_upgrade
  33: salsa::derived::slot::Slot<Q,MP>::read
  34: <salsa::derived::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::try_fetch
  35: salsa::QueryTable<Q>::get
  36: <DB as hir_ty::db::HirDatabase>::impl_trait
  37: hir_ty::method_resolution::TraitImpls::collect_def_map
  38: hir_ty::method_resolution::TraitImpls::trait_impls_in_crate_query
  39: salsa::runtime::Runtime::execute_query_implementation
  40: salsa::derived::slot::Slot<Q,MP>::read_upgrade
  41: salsa::derived::slot::Slot<Q,MP>::read
  42: <salsa::derived::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::try_fetch
  43: <DB as hir_ty::db::HirDatabase>::trait_impls_in_crate::__shim
  44: hir_ty::method_resolution::TraitImpls::trait_impls_in_deps_query
  45: salsa::runtime::Runtime::execute_query_implementation
  46: salsa::derived::slot::Slot<Q,MP>::read_upgrade
  47: salsa::derived::slot::Slot<Q,MP>::read
  48: <salsa::derived::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::try_fetch
  49: <DB as hir_ty::db::HirDatabase>::trait_impls_in_deps::__shim
  50: hir_ty::chalk_db::<impl chalk_solve::RustIrDatabase<hir_ty::interner::Interner> for hir_ty::traits::ChalkContext>::impls_for_trait
  51: chalk_solve::clauses::program_clauses_that_could_match
  52: chalk_recursive::solve::SolveIteration::solve_iteration
  53: chalk_recursive::fixed_point::RecursiveContext<K,V>::solve_goal
  54: chalk_recursive::fixed_point::RecursiveContext<K,V>::solve_root_goal
  55: hir_ty::traits::trait_solve_query
  56: salsa::runtime::Runtime::execute_query_implementation
  57: salsa::derived::slot::Slot<Q,MP>::read_upgrade
  58: salsa::derived::slot::Slot<Q,MP>::read
  59: <salsa::derived::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::try_fetch
  60: salsa::QueryTable<Q>::get
  61: <DB as hir_ty::db::HirDatabase>::trait_solve_query
  62: hir_ty::db::trait_solve_wait
  63: <DB as hir_ty::db::HirDatabase>::trait_solve
  64: hir_ty::method_resolution::implements_trait_unique
  65: hir::Type::impls_fnonce
  66: ide::syntax_highlighting::highlight::highlight_def
  67: ide::syntax_highlighting::highlight::name_like
  68: ide::syntax_highlighting::highlight
  69: salsa::Cancelled::catch
  70: rust_analyzer::handlers::handle_semantic_tokens_range
  71: std::panicking::try
  72: <F as threadpool::FnBox>::call_box

fix: Remap float parts as integers when parsed as indices cc #12149

…s-schievink fix: revert float parsing "fix" to avoid macro-related panics Reverts #12149 and the follow-up fixes, while keeping their tests. #12149 has caused many unexpected panics related to macros, and the fixes for those are not straightforward and further complicate the MBE token conversion logic, which was already fairly hard to follow before these fixes.

Jonas Schievink added 9 commits May 5, 2022 16:27

Remove ast::Literal::token

2d5d16f

Wrap float literals in their own node

502c519

Split float literal tokens at the .

1bc3305

Add tests

2a75549

Add completion test

90bd99f

Wrap floats in token trees in FLOAT_LITERAL node

34dc8e9

Indicate the number of float tokens in the first token

2fe38d3

Maybe everything else *should* have to deal with it

37443eb

Fix rebase fallout

d974a0b

bors merged commit cc9ae2b into rust-lang:master May 5, 2022

jonas-schievink deleted the literally-just-a-literal branch May 5, 2022 15:54

jhgg mentioned this pull request May 6, 2022

rust-analyzer panic Punct { ... } is not a valid punct #12170

Closed

jonas-schievink mentioned this pull request May 6, 2022

Token mapping no longer works for tokens following a float literal #12179

Closed

Veykril mentioned this pull request May 7, 2022

fix: Remap float parts as integers when parsed as indices #12185

Merged

bors added a commit that referenced this pull request May 7, 2022

Auto merge of #12185 - Veykril:parse-fix, r=Veykril

6f6e4ef

fix: Remap float parts as integers when parsed as indices cc #12149

Veetaha mentioned this pull request May 10, 2022

Float parsing regression (repeated panic, basically a crash) #12211

Closed

jonas-schievink mentioned this pull request May 13, 2022

fix: revert float parsing "fix" to avoid macro-related panics #12241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: split float literal tokens at `.` to fix parsing of tuple field accesses #12149

fix: split float literal tokens at `.` to fix parsing of tuple field accesses #12149

jonas-schievink commented May 4, 2022 •

edited

Loading

Veykril commented May 4, 2022

jonas-schievink commented May 4, 2022

Veykril commented May 4, 2022

jonas-schievink commented May 4, 2022

bors commented May 5, 2022

Veykril commented May 5, 2022 •

edited

Loading

jonas-schievink commented May 5, 2022

jonas-schievink commented May 5, 2022

Veykril commented May 5, 2022

jonas-schievink commented May 5, 2022

bors commented May 5, 2022

bors commented May 5, 2022

bors commented May 5, 2022

cynecx commented May 5, 2022 •

edited

Loading

fix: split float literal tokens at . to fix parsing of tuple field accesses #12149

fix: split float literal tokens at . to fix parsing of tuple field accesses #12149

Conversation

jonas-schievink commented May 4, 2022 • edited Loading

Veykril commented May 4, 2022

jonas-schievink commented May 4, 2022

Veykril commented May 4, 2022

jonas-schievink commented May 4, 2022

bors commented May 5, 2022

Veykril commented May 5, 2022 • edited Loading

jonas-schievink commented May 5, 2022

jonas-schievink commented May 5, 2022

Veykril commented May 5, 2022

jonas-schievink commented May 5, 2022

bors commented May 5, 2022

bors commented May 5, 2022

bors commented May 5, 2022

cynecx commented May 5, 2022 • edited Loading

fix: split float literal tokens at `.` to fix parsing of tuple field accesses #12149

fix: split float literal tokens at `.` to fix parsing of tuple field accesses #12149

jonas-schievink commented May 4, 2022 •

edited

Loading

Veykril commented May 5, 2022 •

edited

Loading

cynecx commented May 5, 2022 •

edited

Loading