[redknot] add module type and attribute lookup for some types #11416

plredmond · 2024-05-14T00:56:28Z

Add a module type, ModuleTypeId
Add an attribute lookup method get_member for Type
- Only implemented for ModuleTypeId and ClassTypeId
- Should this be a trait?
  Answer: no
- Uses unwrap, but we should remove that. Maybe add a new variant to QueryError?
  Answer: Return Option<Type> as is done elsewhere
Add infer_definition_type case for Import
Add infer_expr_type case for Attribute
Add a test to exercise these
remove all NOTE/FIXME/TODO after discussing with reviewers

…cally this part: https://github.com/augustelalande/ruff/blob/e1029f1230e51ba75c746bbe5ad0c399e2afa39a/crates/ruff_linter/resources/test/fixtures/pylint/unspecified_encoding.py#L7-L32)

github-actions · 2024-05-14T01:09:54Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

…ment statements in lint_unspecified_encoding

plredmond · 2024-05-24T21:51:05Z

Since you folks were ooo, I added questions that I had as NOTE/TODO/FIXME comments in the diff.

MichaReiser

Looks good to me overall but I let @carljm approve who has more insight on what the actual inference logic should be.

crates/red_knot/src/types.rs

carljm

This is excellent!

crates/red_knot/src/types.rs

carljm · 2024-05-28T03:32:39Z

crates/red_knot/src/types.rs

+                // TODO return a type FF at least one of the unioned-types have the member; return
+                // the union over those member types


Accessing a member of a union should really be a type error if any of the unioned types lack the member. In inference-only mode we won't want to issue this error, though -- I think we should add Unknown to the member type union instead.

Long-term it would be interesting to still return a "guessed" type when only some variants implement the specific member. It could allow for better recovery in some situations (e.g. intelli sense). The API would need to be explicit that the type isn't certain.

Yes, that's actually what my comment proposes, that e.g. if we have x: A | B | C and we access x.foo, and types A and B have a foo attribute but C does not, we would return A.foo | B.foo | Unknown instead of erroring. Effectively this is our best guess of the type, and the union with Unknown is the marker of our uncertainty.

Ok! This approach (returning the union of get_member results for each type in the union, and adding Unknown once if any of result is None) makes more sense than what I was suggesting. I can do that in a followup PR if desired.

One question: Is there any case where get_member of a union type doesn't return a result?

For example, if you have A | B and neither has a .foo attribute, would (A | B).get_member("foo") be Type::Unknown? (sorry abusing syntax here)

Yeah this is a good question, and I don't have a clear answer yet. I need to think through more carefully when we should return None vs Unknown, and how that will interact with inference-only vs type-checking modes.

In principle a nice clean answer would to always use Unknown and never return None. But I worry that this doesn't allow us to distinguish "attribute doesn't exist" from "attribute exists but resolving its type gives us Unknown." But maybe it's ok to not distinguish these from the caller, if we're able to surface diagnostics wherever we create an Unknown. Which I think we should be able to. (It may require passing some kind of diagnostic-sink to all these methods.)

On the other hand I can imagine that in some lint rules we might want to be able to distinguish these cases, without necessarily creating a diagnostic for the missing attribute.

I think that keeping those cases distinct is important at this level, but perhaps it should wait until we have a use case to drive their clarification.

carljm · 2024-05-28T03:35:07Z

crates/red_knot/src/types.rs

+                // TODO return a type IFF all of the intersected-types have the member; but what
+                // type?


In an intersection, the type must be all of the intersected types, so if any of those have the member, it has the member. So we can return a type if any of the types have it; if more than one has it, we should return the intersection of those types.

No types have the member, return None

One type has the member, return the member type (or a singleton intersection containing it?).

Two types have the member, return the intersection of the member types.

Two types have the member, but one type doesn't have the member...? It seems to me that the intersection ought to be empty.

There never a need for singleton intersection (or union), it should always simplify to just the single type.

In the final case, the type that is missing the attribute should just contribute nothing to the intersection, it shouldn't contribute the bottom type (thus making the intersection empty). I would have to think a bit more about the right way to think about this formally, but I'm sure it's the right behavior. A & B & C must be an A and a B and a C, so if only A and B have the attribute, we know the attribute is present and its type is A.foo & B.foo; C contributes no information about its type.

crates/red_knot/src/types.rs

…lass

plredmond · 2024-05-28T17:18:20Z

Thanks for your review. I've made requested changes and asked some questions to describe the follow-up work in comments. Let me know when/if it's ok to merge.

carljm

I think with the one method name change I suggested, this is ready to merge. The union and intersection TODOs don't specify all the details we discussed in those comment threads, but they don't need to, especially since we're not totally clear on all those details yet.

crates/red_knot/src/types.rs

carljm · 2024-05-28T17:47:31Z

Oh there are a couple clippy issues to fix, too.

Looks like you could easily support AnnotatedAssignment the same as Assignment for now (with a TODO to actually look at the annotation) and our Definition handling would cover all variants?

@CARJM

…y adding a case for AnnotatedAssignment per @CARJM

plredmond · 2024-05-28T19:28:06Z

Clippy: rename an unused variable which we'll use after completing the get_member case for unions
Added case to infer_definition_type for AnnotatedAssignment variant which returns Type::Unknown when the value isn't present; includes a TODO to look at the annotation (which presumably will have a type expression)

plredmond · 2024-05-28T19:31:59Z

crates/red_knot/src/types.rs

@@ -529,6 +594,10 @@ impl std::fmt::Display for DisplayType<'_> {
            Type::Never => f.write_str("Never"),
            Type::Unknown => f.write_str("Unknown"),
            Type::Unbound => f.write_str("Unbound"),
+            Type::Module(module_id) => {
+                // NOTE: something like this?: "<module 'module-name' from 'path-from-fileid'>"
+                todo!("{module_id:?}")


I left this as a TODO for now, but could implement it if you tell me what you'd like. It's the type of a module, which doesn't have much info when python prints it out. Probably a bikeshed.

>>> import x >>> x <module 'x' from '/home/asteroid/code/redknot.attrchain/x.py'> >>> type(x) <class 'module'>

I think copying the runtime output (for the module object, not the type, since this is not the general module type but the literal type for a particular module) is reasonable here; totally fine with you either doing it in this PR or leaving it as a todo for later, up to you.

Well, DisplayType only has access to TypeStore. We'd need a SemanticDb to call ModuleTypeId::name and/or a Files to call Files::path on FileId. I don't think I can access either of these in this context. I'll leave it for now.

carljm

Clippy still not happy with that variable name, but once you appease clippy this looks good.

plredmond · 2024-05-28T19:37:24Z

Oh weird. ~~Maybe the CI-clippy is getting some stricter options than my local one.~~[Edit: No, I just forgot to run it] Ty

plredmond added 3 commits May 13, 2024 17:24

infer_definition_type for ImportDefinition

8a09f9f

stub of red-knot unspecified-encoding lint rule (like #11288, specifi…

746b651

…cally this part: https://github.com/augustelalande/ruff/blob/e1029f1230e51ba75c746bbe5ad0c399e2afa39a/crates/ruff_linter/resources/test/fixtures/pylint/unspecified_encoding.py#L7-L32)

debug info so that panic shows what ctor was not matched

0b949c3

plredmond changed the title ~~[redknot] attr chain lookup for unspecified-encoding rule~~ [redknot] module attr chain lookup for unspecified-encoding rule May 14, 2024

plredmond added 13 commits May 16, 2024 17:36

add ModuleTypeId and return one from infer_definition_type

6030d2f

add Module (id) to ModuleTypeId

3470a15

implement member lookup for ModuleTypeId

b7563f0

better todo messages

b88e9ea

import struct

ba234ff

clarify about fmtDisplay

59a9e46

start attribute-lookup implementation

fae67f6

start Call case in infer_expr_type; infer definition types for assign…

280c009

…ment statements in lint_unspecified_encoding

symbol we want to match against

723e283

test of infer_expr_type Attribute case

baf8cb3

remove lint stubs

8d967e1

a comment

d7268f7

remove infer_expr_type for call

6a94690

plredmond changed the title ~~[redknot] module attr chain lookup for unspecified-encoding rule~~ [redknot] module type, attribute type lookup May 24, 2024

plredmond changed the title ~~[redknot] module type, attribute type lookup~~ [redknot] module type, attribute lookup May 24, 2024

plredmond changed the title ~~[redknot] module type, attribute lookup~~ [redknot] add module type and attribute lookup for some types May 24, 2024

plredmond marked this pull request as ready for review May 24, 2024 21:49

plredmond requested review from carljm and MichaReiser May 24, 2024 21:49

MichaReiser added the internal An internal refactor or improvement label May 27, 2024

MichaReiser reviewed May 27, 2024

View reviewed changes

crates/red_knot/src/types.rs Outdated Show resolved Hide resolved

crates/red_knot/src/types.rs Outdated Show resolved Hide resolved

crates/red_knot/src/types.rs Show resolved Hide resolved

carljm reviewed May 28, 2024

View reviewed changes

plredmond added 2 commits May 28, 2024 09:47

remove NOTE from ModuleTypeId methods

25395b6

ClassTypeId method get_member

71dd471

plredmond added 3 commits May 28, 2024 09:55

remove unwrap in Type::get_member and simplify cases for module and c…

e028ac5

…lass

correct typo in comment

feac185

explain TODO for union of get_member

51594b6

plredmond requested a review from carljm May 28, 2024 17:17

clarification about get_member on intersections

becb724

carljm approved these changes May 28, 2024

View reviewed changes

crates/red_knot/src/types.rs Outdated Show resolved Hide resolved

crates/red_knot/src/types.rs Outdated Show resolved Hide resolved

plredmond added 4 commits May 28, 2024 12:16

rename ClassTypeId::get_member to get_class_member

95b46c1

infer_definition_type: move case for Import above case for ImportFrom

1bde5d6

complete matching over Definition variants in infer_definition_type b…

affbc3c

…y adding a case for AnnotatedAssignment per @CARJM

clippy -- fix unused variable

8b2e202

plredmond commented May 28, 2024

View reviewed changes

carljm approved these changes May 28, 2024

View reviewed changes

appease clippy

a200b06

plredmond merged commit 9a3b9f9 into main May 28, 2024
19 checks passed

plredmond deleted the redknot.attrchain branch May 28, 2024 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[redknot] add module type and attribute lookup for some types #11416

[redknot] add module type and attribute lookup for some types #11416

plredmond commented May 14, 2024 •

edited

github-actions bot commented May 14, 2024 •

edited

plredmond commented May 24, 2024

MichaReiser left a comment

carljm left a comment

carljm May 28, 2024

MichaReiser May 28, 2024

carljm May 28, 2024

plredmond May 28, 2024 •

edited

carljm May 28, 2024

carljm May 28, 2024

plredmond May 28, 2024

carljm May 28, 2024

plredmond May 28, 2024

carljm May 28, 2024

plredmond commented May 28, 2024 •

edited

carljm left a comment

carljm commented May 28, 2024 •

edited

plredmond commented May 28, 2024

plredmond May 28, 2024

carljm May 28, 2024 •

edited

plredmond May 28, 2024

carljm left a comment

plredmond commented May 28, 2024 •

edited

		// TODO return a type FF at least one of the unioned-types have the member; return
		// the union over those member types

		// TODO return a type IFF all of the intersected-types have the member; but what
		// type?

[redknot] add module type and attribute lookup for some types #11416

[redknot] add module type and attribute lookup for some types #11416

Conversation

plredmond commented May 14, 2024 • edited

github-actions bot commented May 14, 2024 • edited

ruff-ecosystem results

Linter (stable)

Linter (preview)

plredmond commented May 24, 2024

MichaReiser left a comment

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plredmond May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plredmond commented May 28, 2024 • edited

carljm left a comment

Choose a reason for hiding this comment

carljm commented May 28, 2024 • edited

plredmond commented May 28, 2024

Choose a reason for hiding this comment

carljm May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

plredmond commented May 28, 2024 • edited

plredmond commented May 14, 2024 •

edited

github-actions bot commented May 14, 2024 •

edited

`ruff-ecosystem` results

plredmond May 28, 2024 •

edited

plredmond commented May 28, 2024 •

edited

carljm commented May 28, 2024 •

edited

carljm May 28, 2024 •

edited

plredmond commented May 28, 2024 •

edited