[red-knot] function parameter types

[carljm]

Summary

Inferred and declared types for function parameters, in the function body scope.

Fixes #13693.

Test Plan

Added mdtests.

Co-authored By: Alex Waygood AlexWaygood@Gmail.com

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

Unrelated to this PR and I know you're aware of it.... I find it very confusing as a user that this infers to Literal[1] | Unknown, and I'd have a hard time explaining it to any colleague of why and even what it means.

[sharkdp]

I know this is not the answer you are looking for, but let me try to explain the current/intended behavior, to see if my own understanding is correct.

We want to follow a philosophy where a perfectly valid un-annotated program should ideally not lead us to issue any diagnostics.

Consider this perfectly reasonable and working (untyped) program:

def f(x, a=0):
    return 2 * x * x + a


f(2.0, 1.2)

pyright issues a diagnostic here (Argument of type "float" cannot be assigned to parameter "a" of type "int") because it infers a (too) narrow parameter type of int.

We infer Unknown | Literal[0] instead, which does not raise any diagnostic. We also wouldn't raise a diagnostic if you were to call f(2.0, "foo"), but you can always achieve this by annotating your function parameter appropriately (e.g. as float). You can opt into more safety.

On the other hand of the spectrum, consider this non-crashing — but obviously error-prone — program:

def f(name, age=None):
    # lots of code
    print(f"Next year, {name} will turn {age+1}")

f("Max", 20)

mypy does not raise any diagnostic here, because it infers a (too) wide parameter type of Any for age.

We infer Unknown | NoneType instead. The meaning of this type is: an unknown set of Python objects, but at least as large as the set represented by NoneType (i.e. {None}). This allows us to issue a Operator + not supported diagnostic (not yet but eventually; only works for operators like in so far). In other words: if we see age=None, we say: we don't know what kind of objects can be passed in for the age parameter, but you are clearly showing us that None is one possibility, so we better make sure that all operations on age are compatible with NoneType.

[carljm]

I couldn't say it better than @sharkdp did, in terms of explaining what this type means and why we need it.

I understand the real issue here is that users who haven't gotten such a good explanation will likely be confused by these types, because other type checkers don't use them much. I'm not yet sure how to reconcile this. It may be that we can find a more intuitive display notation, or it may be that this is just something we have to address via documentation.

[MichaReiser]

Yeah. I just see myself struggling with it and am worried that people will accept that Red Knot shows these weird inferred types and maybe even understand why but without fully understanding what it means

[MichaReiser]

Just an understand question. Why are we calling definition_expression_ty over infer_standalone_expression?

Do we need to merge the diagnostics from the definition_expression_ty ?

[carljm]

Why are we calling definition_expression_ty over infer_standalone_expression?

Because parameter default values are not marked as standalone expressions in the semantic index builder. We minimize the expressions that are marked as standalone expressions, to reduce ingredient counts. In this case we don't need them to be standalone expressions, we can use definition_expression_ty to find the right scope, infer that scope, and pull the expression type out.

Do we need to merge the diagnostics from the definition_expression_ty ?

No, in fact we shouldn't. Function parameter definitions are a little odd, in that the definition is part of the function body scope (it defines a symbol in the function body), but the expressions whose type we need here (the annotation and the default) are not part of the function body scope, they are part of the outer scope (or possibly two different outer scopes: the defaults are always in the scope the function is defined in, the annotations might be in the intervening type-parameters scope, for a generic function.) Part of the functionality of definition_expression_ty is that it can handle inferring expressions that are in some other scope. But we shouldn't try to merge diagnostics that belong to a different scope.

[carljm]

I realized here that definition_expression_ty is kind of over-powered for this use case, since in these cases we know for sure that the expression is not in the same scope as the definition. So I've instead introduced an expression_ty that just looks up the right scope for the expression, infers that scope, and pulls out the expression type.

[dhruvmanila]

Out of curiosity, is there a use-case for having expression_ty as a standalone function instead of adding it as a method on TypeInference? For the latter, we could add an assert to make sure that the expression is not from the same scope as the one that's currently being inferred which is available via the self.scope method:

ruff/crates/red_knot_python_semantic/src/types/infer.rs

Lines 393 to 395 in 56a631a

	fn scope(&self) -> ScopeId<'db> {
	self.types.scope
	}

[carljm]

That's a good idea, I'll put up a PR for this.

[carljm]

#14879

[AlexWaygood]

🥳

[MichaReiser]

@AlexWaygood are you nitpicking your own code? 😆 (just kidding, these are great improvements and not nits!)

[carljm]

these are great improvements and not nits!

I think it's fair to say that adding backticks to a TODO comment is indeed a nit 😆 If that's not a nit, I don't know what would be. But I don't mind some nits :)

[carljm]

Going to go ahead and merge this rather than wait until Monday. I did make some significant changes in response to review; happy to push a follow-up PR if there are comments on the new changes.