Nit: it would be nice if we can avoid allocating a hash set if we know that there are fewer than 2 keyword arguments.

let mut keyword_arguments = args.keywords.iter().filter_map(|key| &key.arg).peekable();

let Some(first) = keyword_arguments.next() else {
	return;
};


if keyword_arguments.peek().is_none() {
	return;
}

let mut unique_args = FxHashSet::default();
unique_args.insert(first);

for arg in keyword_arguments {
	if !unique_keyword_args.insert(ident) {
	  let range = ident.range();
	  // test_err duplicate_keyword_args
	  // def foo(x): ...
	  // foo(x=1, x=2)
	  // def baz(x, y, z): ...
	  // baz(x, y=1, z=3, y=4)
	
	  // test_ok non_duplicate_keyword_args
	  // def foo(x): ...
	  // foo(x=1)
	  // def bar(x, y, z): ...
	  // foo(x="a", y=1, z=True)
	  Self::add_error(
	      ctx,
	      SemanticSyntaxErrorKind::DuplicateKeywordArgs(ident.to_string()),
	      range,
	  );
	}
}

Another alternative would be that we make the FxHashSet a field on the visitor and reuse it (we'd have to make sure that we always call clear()). This would ensure that we only allocate at most one hash set. I think I'd prefer that (because we could use it for other checks too)

yeah that makes sense! By visitor do you mean SemanticSyntaxChecker or one of InvalidExpressionVisitor, ReboundComprehensionVisitor or MatchPatternVisitor?

I think Micha means SemanticSyntaxChecker, which would also require passing a &mut self parameter to this function.

Yeah that's what I was wondering, in this case if we wanted to avoid clone, we'd have a HashSet<&Identifier> which means adding a lifetime to SemanticSyntaxChecker and most of the places it's used. The borrow checker was giving me some trouble but I'll take another shot at it

I got it to compile using unsafe like this

    fn duplicate_keyword_args<Ctx: SemanticSyntaxContext>(
        &'a mut self,
        args: &ast::Arguments,
        ctx: &Ctx,
    ) {
        let args: &'a ast::Arguments = unsafe { std::mem::transmute(args) };

        for key in &args.keywords {
            if let Some(ident) = &key.arg {
                if !self.keyword_args.insert(ident) {
                    let range = ident.range();

                    Self::add_error(
                        ctx,
                        SemanticSyntaxErrorKind::DuplicateKeywordArgs(ident.to_string()),
                        range,
                    );
                }
            }
        }

        self.keyword_args.clear();
    }

The problem is that the borrow checker doesn't understand that calling clear() at the end of the function invalidates the references to &Arguments stored in the HashSet, and since mutable references are invariant this leads it to believe that &Arguments must outlive &mut self. We know that isn't necessary since any reference to &Arguments will always be cleared at the end of the function, so I used unsafe to coerce the &Arguments lifetime into the same as &mut self.

The problem with this approach is that

1. We are using unsafe, even though it seems pretty "safe" here, I'm not sure what ruff's stance to using unsafe is.
2. This raises lifetime errors in tests/fixtures.rs, these can probably be solved but would still require some work.

The other options I can think of are:

1. Leaving it as is, allocating and deallocating a hashset on every function call
2. Reuse the hashset, but make it a FxHashSet<ast::Identifier> and call clone() on every argument

Both of these options allocate more memory than necessary, though I'm not sure which is costlier (though I think cloneing would most times be cheaper I haven't actually tested it).

Let me know what your thoughts are.

Funny enough I did try something similar to this but couldn't get it to work, but now it worked, thanks! Now I should just need to add some lifetime annotations where SemanticSyntaxChecker is used elsewhere in the code base

Yeah I mean I can't do it, changing this lifetime raised a bunch of lifetime errors (close to 80) on other parts of the codebase, mostly the Visitor implementations on with_semantic_checker calls. Maybe there's a way to fix them using some specific annotations, but I couldn't figure it out. So I'll leave it like this for now.

Thanks for trying. I think this is okay. codspeed is only showing a few ~1% performance regressions, which I think is pretty typical for any new rule. I think we just need to resolve the question about the duplicate diagnostic (and the clippy errors) and then we can land this.

It may not overlap 100%, but there is an open issue in ty (astral-sh/ty#119) to avoid emitting type inference diagnostics in the presence of syntax errors, which might be enough to ignore the duplication issue for the sake of this PR.

I think this goes somewhat beyond #119. At least, what I had in mind for it. The goal for #119 is to avoid type checker errors for nodes that have non semantic or version related syntax errors.