From cc26d3545c232fdd64e52af92eb74e16853ee142 Mon Sep 17 00:00:00 2001 From: Kyle Into Date: Tue, 14 Jan 2025 07:42:17 -0800 Subject: [PATCH] allow duplicate class keywords Summary: storing class metadata keywords as a `Map` limits them to a single keyword per name. But Python syntax allows duplicate keywords with the same name. this diff: - changes the storage from a map to a vec in order to allow multiple with conflicting names - changes get_class_keyword api to get_class_keywords allowing >1 keyword Reviewed By: stroxler, ndmitchell Differential Revision: D68124675 fbshipit-source-id: 327172dd7f29e6ac9d868dd47fb3c45da5b1004d --- pyre2/pyre2/bin/alt/classes.rs | 4 ++-- pyre2/pyre2/bin/binding/binding.rs | 7 +++---- pyre2/pyre2/bin/binding/bindings.rs | 14 ++----------- pyre2/pyre2/bin/test/class_keywords.rs | 28 ++++++++++--------------- pyre2/pyre2/bin/types/class_metadata.rs | 7 +++---- 5 files changed, 21 insertions(+), 39 deletions(-) diff --git a/pyre2/pyre2/bin/alt/classes.rs b/pyre2/pyre2/bin/alt/classes.rs index 41701356b8c..de6c292053f 100644 --- a/pyre2/pyre2/bin/alt/classes.rs +++ b/pyre2/pyre2/bin/alt/classes.rs @@ -315,7 +315,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { &self, cls: &Class, bases: &[Expr], - keywords: &SmallMap, + keywords: &[(Name, Expr)], ) -> ClassMetadata { let bases_with_metadata: Vec<_> = bases .iter() @@ -330,7 +330,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { _ => None, }) .collect(); - let (metaclasses, keywords): (Vec<_>, SmallMap<_, _>) = + let (metaclasses, keywords): (Vec<_>, Vec<(_, _)>) = keywords.iter().partition_map(|(n, x)| match n.as_str() { "metaclass" => Either::Left(x), _ => Either::Right((n.clone(), self.expr(x, None))), diff --git a/pyre2/pyre2/bin/binding/binding.rs b/pyre2/pyre2/bin/binding/binding.rs index 3df2a1a2528..8e4c96397dd 100644 --- a/pyre2/pyre2/bin/binding/binding.rs +++ b/pyre2/pyre2/bin/binding/binding.rs @@ -22,7 +22,6 @@ use ruff_python_ast::StmtFunctionDef; use ruff_python_ast::TypeParams; use ruff_text_size::Ranged; use ruff_text_size::TextRange; -use starlark_map::small_map::SmallMap; use starlark_map::small_set::SmallSet; use static_assertions::assert_eq_size; @@ -50,7 +49,7 @@ assert_eq_size!(KeyLegacyTypeParam, [usize; 1]); assert_eq_size!(Binding, [usize; 9]); assert_eq_size!(BindingAnnotation, [usize; 9]); -assert_eq_size!(BindingClassMetadata, [usize; 8]); +assert_eq_size!(BindingClassMetadata, [usize; 7]); assert_eq_size!(BindingClassField, [usize; 15]); assert_eq_size!(BindingLegacyTypeParam, [u32; 1]); @@ -657,9 +656,9 @@ impl DisplayWith for BindingClassField { /// /// The `Key` points to the definition of the class. /// The `Vec` contains the base classes from the class header. -/// The `SmallMap` contains the class keywords from the class header. +/// The `Vec<(Name, Expr)>` contains the class keywords from the class header. #[derive(Clone, Debug)] -pub struct BindingClassMetadata(pub Idx, pub Vec, pub SmallMap); +pub struct BindingClassMetadata(pub Idx, pub Vec, pub Vec<(Name, Expr)>); impl DisplayWith for BindingClassMetadata { fn fmt(&self, f: &mut fmt::Formatter<'_>, ctx: &Bindings) -> fmt::Result { diff --git a/pyre2/pyre2/bin/binding/bindings.rs b/pyre2/pyre2/bin/binding/bindings.rs index b90f39d8957..13507a40711 100644 --- a/pyre2/pyre2/bin/binding/bindings.rs +++ b/pyre2/pyre2/bin/binding/bindings.rs @@ -1192,21 +1192,11 @@ impl<'a> BindingsBuilder<'a> { base }); - let mut keywords = SmallMap::new(); + let mut keywords = Vec::new(); x.keywords().iter().for_each(|keyword| { if let Some(name) = &keyword.arg { self.ensure_expr(&keyword.value); - if keywords - .insert(name.id.clone(), keyword.value.clone()) - .is_some() - { - // TODO(stroxler) We should use a Vec rather than a Map in the binding - // so that we can still type check the values associated with - // duplicate keywords. - // - // For now, we get a type error from the parser but never - // check the expression. - } + keywords.push((name.id.clone(), keyword.value.clone())); } else { self.error( keyword.range(), diff --git a/pyre2/pyre2/bin/test/class_keywords.rs b/pyre2/pyre2/bin/test/class_keywords.rs index 8675cf6cc1c..367dff2637a 100644 --- a/pyre2/pyre2/bin/test/class_keywords.rs +++ b/pyre2/pyre2/bin/test/class_keywords.rs @@ -5,28 +5,27 @@ * LICENSE file in the root directory of this source tree. */ -use ruff_python_ast::name::Name; - use crate::module::module_name::ModuleName; use crate::state::state::State; use crate::test::mro::get_class_metadata; use crate::test::util::mk_state; use crate::testcase; -use crate::testcase_with_bug; use crate::types::class::ClassType; use crate::types::literal::Lit; use crate::types::types::Type; -fn get_class_keyword( +fn get_class_keywords( class_name: &str, keyword_name: &str, module_name: ModuleName, state: &State, -) -> Option { +) -> Vec { get_class_metadata(class_name, module_name, state) .keywords() - .get(&Name::new(keyword_name)) - .cloned() + .iter() + .filter(|(name, _type)| name.as_str() == keyword_name) + .map(|(_, ty)| ty.clone()) + .collect() } fn get_metaclass(class_name: &str, module_name: ModuleName, state: &State) -> Option { @@ -43,10 +42,10 @@ class A(foo=True): pass "#, ); assert_eq!( - get_class_keyword("A", "foo", module_name, &state), - Some(Type::Literal(Lit::Bool(true))), + get_class_keywords("A", "foo", module_name, &state), + vec![Type::Literal(Lit::Bool(true))], ); - assert_eq!(get_class_keyword("A", "bar", module_name, &state), None); + assert_eq!(get_class_keywords("A", "bar", module_name, &state), vec![]); } #[test] @@ -124,15 +123,10 @@ class A(B0, B1): # E: Class `A` has metaclass `M0` which is not a subclass of "#, ); -testcase_with_bug!( - r#" -TODO(stroxler): Ideally we would not only report a duplicate keyword here, -but also catch all type errors even in duplicate keywords. Here we miss a type -error in the first value for `foo` because it gets overwritten. - "#, +testcase!( test_duplicate_class_keyword, r#" -class A(foo="x" + 5, foo=True): # E: Parse error: Duplicate keyword argument "foo" +class A(foo="x" + 5, foo=True): # E: Parse error: Duplicate keyword argument "foo" # E: EXPECTED Literal[5] <: str pass "#, ); diff --git a/pyre2/pyre2/bin/types/class_metadata.rs b/pyre2/pyre2/bin/types/class_metadata.rs index 2ee6ca70a59..0fa7e4db920 100644 --- a/pyre2/pyre2/bin/types/class_metadata.rs +++ b/pyre2/pyre2/bin/types/class_metadata.rs @@ -12,7 +12,6 @@ use std::iter; use std::sync::Arc; use ruff_python_ast::name::Name; -use starlark_map::small_map::SmallMap; use vec1::Vec1; use crate::error::collector::ErrorCollector; @@ -37,7 +36,7 @@ impl ClassMetadata { cls: &Class, bases_with_metadata: Vec<(ClassType, Arc)>, metaclass: Option, - keywords: SmallMap, + keywords: Vec<(Name, Type)>, errors: &ErrorCollector, ) -> ClassMetadata { ClassMetadata( @@ -57,7 +56,7 @@ impl ClassMetadata { } #[allow(dead_code)] // This is used in tests now, and will be needed later in production. - pub fn keywords(&self) -> &SmallMap { + pub fn keywords(&self) -> &[(Name, Type)] { &self.2.0 } @@ -98,7 +97,7 @@ impl Display for Metaclass { /// The `metaclass` keyword is not included, since we store the metaclass /// separately as part of `ClassMetadata`. #[derive(Clone, Debug, PartialEq, Eq, Default)] -struct Keywords(SmallMap); +struct Keywords(Vec<(Name, Type)>); impl Display for Keywords { fn fmt(&self, f: &mut Formatter) -> fmt::Result {