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

@@ -315,7 +315,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
         &self,
         cls: &Class,
         bases: &[Expr],
-        keywords: &SmallMap<Name, Expr>,
+        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))),

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<Bindings> for BindingClassField {
 ///
 /// The `Key` points to the definition of the class.
 /// The `Vec<Expr>` contains the base classes from the class header.
-/// The `SmallMap<Name, Expr>` 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<Key>, pub Vec<Expr>, pub SmallMap<Name, Expr>);
+pub struct BindingClassMetadata(pub Idx<Key>, pub Vec<Expr>, pub Vec<(Name, Expr)>);
 
 impl DisplayWith<Bindings> for BindingClassMetadata {
     fn fmt(&self, f: &mut fmt::Formatter<'_>, ctx: &Bindings) -> fmt::Result {

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(),

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<Type> {
+) -> Vec<Type> {
     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<ClassType> {
@@ -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
 "#,
 );

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<ClassMetadata>)>,
         metaclass: Option<ClassType>,
-        keywords: SmallMap<Name, Type>,
+        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<Name, Type> {
+    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<Name, Type>);
+struct Keywords(Vec<(Name, Type)>);
 
 impl Display for Keywords {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {