rework how variable/id ambiguity works

Author: nikomatsakis

crates/formality-core/src/parse.rs

@@ -81,6 +81,11 @@ pub struct SuccessfulParse<'t, T> {
     /// to `parse_variant`.
     precedence: Precedence,
 
+    /// Did this result from parsing an in-scope variable? If so,
+    /// we prefer this parse to other parses that consumed the same set of
+    /// input tokens.
+    is_in_scope_var: bool,
+
     /// The value produced.
     value: T,
 }
@@ -99,6 +104,7 @@ impl<'t, T> SuccessfulParse<'t, T> {
             text: self.text,
             reductions: self.reductions,
             precedence: self.precedence,
+            is_in_scope_var: self.is_in_scope_var,
             value: op(self.value),
         }
     }
@@ -114,6 +120,7 @@ where
             text: term.text,
             reductions: term.reductions,
             precedence: term.precedence,
+            is_in_scope_var: term.is_in_scope_var,
             value: term.value.upcast(),
         }
     }

crates/formality-core/src/parse/parser.rs

@@ -153,6 +153,7 @@ where
     current_text: &'t str,
     reductions: Vec<&'static str>,
     is_cast_variant: bool,
+    is_in_scope_var: bool,
 
     /// A variant is 'committed' when we have seen enough success
     is_committed: bool,
@@ -232,6 +233,20 @@ where
         })
     }
 
+    pub fn parse_variant_variable(
+        &mut self,
+        variant_name: &'static str,
+        variant_precedence: Precedence,
+        kind: L::Kind,
+    ) where
+        CoreVariable<L>: Upcast<T>,
+    {
+        Self::parse_variant(self, variant_name, variant_precedence, |p| {
+            p.mark_as_in_scope_var();
+            p.variable_of_kind(kind)
+        })
+    }
+
     /// Parses a single variant for this nonterminal.
     /// The name of the variant is significant and will be tracked as part of the list of reductions.
     /// The precedence is part of how we resolve conflicts: if there are two successful parses with distinct precedence, higher precedence wins.
@@ -272,7 +287,7 @@ where
 
         match result {
             Ok(value) => {
-                // Subtle: for cast variants, don't record the variant name in the reduction lits,
+                // Subtle: for cast variants, don't record the variant name in the reduction list,
                 // as it doesn't carry semantic weight. See `mark_as_cast_variant` for more details.
                 if !active_variant.is_cast_variant {
                     active_variant.reductions.push(variant_name);
@@ -282,6 +297,7 @@ where
                     text: active_variant.current_text,
                     reductions: active_variant.reductions,
                     precedence: variant_precedence,
+                    is_in_scope_var: active_variant.is_in_scope_var,
                     value: value.upcast(),
                 });
                 tracing::trace!("success: {:?}", self.successes.last().unwrap());
@@ -369,6 +385,9 @@ where
         }
 
         has_prefix(&s_i.reductions, &s_j.reductions)
+            || (s_i.is_in_scope_var
+                && !s_j.is_in_scope_var
+                && skip_whitespace(s_i.text) == skip_whitespace(s_j.text))
     }
 }
 
@@ -385,6 +404,7 @@ where
             current_text: start_text,
             reductions: vec![],
             is_cast_variant: false,
+            is_in_scope_var: false,
             is_committed: true,
         }
     }
@@ -481,6 +501,16 @@ where
         self.is_cast_variant = true;
     }
 
+    /// Indicates that this variant is parsing *only* an in-scope variable.
+    /// We have special treatment for disambiguation such that if a variant marked
+    /// as parsing an "in-scope variable" and some other variant both consume the
+    /// same part of the input string, we prefer the variable. This method is automatically
+    /// invoked on `#[variable]` variants by the generated parser code and you generally
+    /// wouldn't want to call it yourself.
+    pub fn mark_as_in_scope_var(&mut self) {
+        self.is_in_scope_var = true;
+    }
+
     /// Expect *exactly* the given text (after skipping whitespace)
     /// in the input string. Reports an error if anything else is observed.
     /// In error case, consumes only whitespace.
@@ -569,6 +599,7 @@ where
             reductions: vec![],
             scope: self.scope,
             is_cast_variant: false,
+            is_in_scope_var: false,
             is_committed: true,
         };
 
@@ -629,6 +660,7 @@ where
             current_text: self.current_text,
             reductions: vec![],
             is_cast_variant: false,
+            is_in_scope_var: false,
             is_committed: true,
         };
         let result = op(&mut av);
@@ -637,21 +669,6 @@ where
         result
     }
 
-    /// Returns an error if an in-scope variable name is found.
-    /// The derive automatically inserts calls to this for all other variants
-    /// if any variant is declared `#[variable]`.
-    pub fn reject_variable(&self) -> Result<(), Set<ParseError<'t>>> {
-        self.reject::<CoreVariable<L>>(
-            |p| p.variable(),
-            |var| {
-                ParseError::at(
-                    self.current_text,
-                    format!("found unexpected in-scope variable {:?}", var),
-                )
-            },
-        )
-    }
-
     /// Parses the next identifier as a variable in scope.
     ///
     /// **NB:** This departs from the limits of context-free
@@ -873,6 +890,7 @@ where
             text,
             reductions,
             precedence: _,
+            is_in_scope_var: _,
             value,
         } = left_recursion::recurse(self.current_state(), || op(self.scope, self.current_text))?;
 

crates/formality-macros/src/parse.rs

@@ -7,7 +7,7 @@ use syn::{spanned::Spanned, Attribute};
 use synstructure::BindingInfo;
 
 use crate::{
-    attrs::{has_cast_attr, has_variable_attr, precedence, variable},
+    attrs::{has_cast_attr, precedence, variable},
     spec::{self, FieldMode, FormalitySpec},
     variable::Variable,
 };
@@ -40,17 +40,10 @@ pub(crate) fn derive_parse_with_spec(
         }
     }
 
-    // Determine whether *any* variant is marked as `#[variable]`.
-    // If so, all *other* variants will automatically reject in-scope variable names.
-    let any_variable_variant = s
-        .variants()
-        .iter()
-        .any(|v| has_variable_attr(v.ast().attrs));
-
     let mut parse_variants = TokenStream::new();
     for variant in s.variants() {
         let variant_name = Literal::string(&format!("{}::{}", s.ast().ident, variant.ast().ident));
-        let v = parse_variant(variant, external_spec, any_variable_variant)?;
+        let v = parse_variant(variant, external_spec)?;
         let precedence = precedence(variant.ast().attrs)?.expr();
         parse_variants.extend(quote_spanned!(
             variant.ast().ident.span() =>
@@ -76,19 +69,11 @@ pub(crate) fn derive_parse_with_spec(
 fn parse_variant(
     variant: &synstructure::VariantInfo,
     external_spec: Option<&FormalitySpec>,
-    any_variable_variant: bool,
 ) -> syn::Result<TokenStream> {
     let ast = variant.ast();
     let variable_attr = variable(ast.attrs)?;
     let mut stream = TokenStream::default();
 
-    // If there are variable variants -- but this is not one -- then we always begin by rejecting
-    // variable names. This avoids ambiguity in a case like `for<ty X> { X : Debug }`, where we
-    // want to parse `X` as a type variable.
-    if any_variable_variant && variable_attr.is_none() {
-        stream.extend(quote_spanned!(ast.ident.span() => __p.reject_variable()?;));
-    }
-
     // When invoked like `#[term(foo)]`, use the spec from `foo`
     if let Some(spec) = external_spec {
         return parse_variant_with_attr(variant, spec, stream);
@@ -124,6 +109,7 @@ fn parse_variant(
         let construct = variant.construct(field_ident);
         stream.extend(quote_spanned! {
             ast.ident.span() =>
+            __p.mark_as_in_scope_var();
             let #v0 = __p.variable_of_kind(#kind)?;
             Ok(#construct)
         });

crates/formality-types/src/grammar/ty/parse_impls.rs

@@ -123,9 +123,7 @@ fn parse_parameters<'t>(
 impl CoreParse<Rust> for ConstData {
     fn parse<'t>(scope: &Scope<Rust>, text: &'t str) -> ParseResult<'t, Self> {
         Parser::multi_variant(scope, text, "ConstData", |parser| {
-            parser.parse_variant("Variable", Precedence::default(), |p| {
-                p.variable_of_kind(ParameterKind::Const)
-            });
+            parser.parse_variant_variable("Variable", Precedence::default(), ParameterKind::Const);
 
             parser.parse_variant_cast::<Bool>(Precedence::default());
 

tests/parser_var_id_ambiguity.rs

@@ -0,0 +1,114 @@
+//! Test for how we handle ambiguity between a variable and an identifier in the parser.
+//! This can't be part of parser-torture-tests because it requires settting up a custom variable.
+
+use std::sync::Arc;
+
+// Default language for our crate
+use formality_core::{language::HasKind, term};
+use ptt::grammar::{Binder, BoundVar, ExistentialVar, UniversalVar, Variable};
+use ptt::FormalityLang;
+
+formality_core::declare_language! {
+    mod ptt {
+        const NAME = "PTT";
+        type Kind = crate::Kind;
+        type Parameter = crate::Parameter;
+        const BINDING_OPEN = '<';
+        const BINDING_CLOSE = '>';
+        const KEYWORDS = [
+        ];
+    }
+}
+
+#[term]
+#[derive(Copy)]
+pub enum Kind {
+    Ty,
+}
+
+#[term]
+pub enum Parameter {
+    #[cast]
+    Ty(Ty),
+}
+
+#[term]
+pub enum Ty {
+    #[variable(Kind::Ty)]
+    Variable(Variable),
+
+    #[cast]
+    Id(Id),
+
+    #[grammar($v0 :: $v1)]
+    Assoc(Arc<Ty>, Id),
+}
+
+formality_core::id!(Id);
+
+formality_core::cast_impl!((BoundVar) <: (Variable) <: (Ty));
+formality_core::cast_impl!((ExistentialVar) <: (Variable) <: (Ty));
+formality_core::cast_impl!((UniversalVar) <: (Variable) <: (Ty));
+formality_core::cast_impl!((Variable) <: (Ty) <: (Parameter));
+formality_core::cast_impl!((BoundVar) <: (Ty) <: (Parameter));
+formality_core::cast_impl!((ExistentialVar) <: (Ty) <: (Parameter));
+formality_core::cast_impl!((UniversalVar) <: (Ty) <: (Parameter));
+
+impl HasKind<FormalityLang> for Parameter {
+    fn kind(&self) -> formality_core::language::CoreKind<FormalityLang> {
+        match self {
+            Parameter::Ty(_) => Kind::Ty,
+        }
+    }
+}
+
+#[test]
+fn parse_var() {
+    let value: Binder<Ty> = ptt::term("<ty T> T");
+    expect_test::expect![[r#"
+        Binder {
+            kinds: [
+                Ty,
+            ],
+            term: Variable(
+                ^ty0_0,
+            ),
+        }
+    "#]]
+    .assert_debug_eq(&value);
+}
+
+#[test]
+fn parse_id() {
+    let value: Binder<Ty> = ptt::term("<ty T> i32");
+    expect_test::expect![[r#"
+        Binder {
+            kinds: [
+                Ty,
+            ],
+            term: Id(
+                i32,
+            ),
+        }
+    "#]]
+    .assert_debug_eq(&value);
+}
+
+#[test]
+fn parse_assoc() {
+    let value: Binder<Ty> = ptt::term("<ty T> i32::T");
+    expect_test::expect![[r#"
+        Binder {
+            kinds: [
+                Ty,
+            ],
+            term: Assoc(
+                Id(
+                    i32,
+                ),
+                T,
+            ),
+        }
+    "#]]
+    .assert_debug_eq(&value);
+}