Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add diagnostic for integer division by zero #13576

Merged
merged 1 commit into from
Sep 30, 2024
Merged

Add diagnostic for integer division by zero #13576

merged 1 commit into from
Sep 30, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Sep 30, 2024

Adds a diagnostic for division by the integer zero in //, /, and %.

Doesn't handle <int> / 0.0 because we don't track the values of float literals.

@zanieb zanieb added the red-knot Multi-file analysis & type inference label Sep 30, 2024
zanieb added a commit that referenced this pull request Sep 30, 2024
While working on #13576 I noticed
that it was really hard to tell which assertion failed in some of these
test cases. This could be expanded to elsewhere, but I've heard this
test suite format won't be around for long?
Copy link
Contributor

github-actions bot commented Sep 30, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

&[
"Cannot divide type 'Literal[1]' by zero.",
"Cannot divide type 'Literal[2]' by zero.",
"Cannot divide type 'Literal[3]' by zero.",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could want a special message for modulo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modulo requires division to calculate, so I think the error message is accurate and adequate as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but not all beginners may realize that! Seems fine unless we receive significant feedback though.

Copy link
Member

@AlexWaygood AlexWaygood Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree with @zanieb here -- the current message is entirely accurate, but I think even your average Python user (letalone a beginner) doesn't necessarily think about modulo operations in terms of their desugared sub-operations, so I think encountering this error message here might be pretty surprising to a lot of our users. It's definitely not the biggest issue in the world, but I think a better error message here would be really worthwhile (and I actually think it's probably better to do it now before we forget about it)

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
@carljm
Copy link
Contributor

carljm commented Sep 30, 2024

Doesn't handle <float> / 0 because it's not clear to me what the proper pattern is to match that type. I presume I would check for equality after looking up the builtin?

I think it should be something like a match pattern for Type::Instance(cls) if cls.is_stdlib_symbol(db, "builtins", "float") to match the float type; similar for int.

@zanieb zanieb force-pushed the zb/rk-int-div-zero branch 2 times, most recently from 8b8a901 to d7c95fa Compare September 30, 2024 22:14
@zanieb zanieb requested a review from carljm September 30, 2024 22:14
@zanieb
Copy link
Member Author

zanieb commented Sep 30, 2024

I made some relatively significant changes per the review. Thanks again Carl.

Comment on lines -2314 to +2342
.unwrap_or(Type::Todo),
.unwrap_or_else(|| builtins_symbol_ty(self.db, "int").to_instance(self.db)),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion, division by zero (and overflows, which should never occur here) don't matter when determining the inferred type.

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
&[
"Cannot divide type 'Literal[1]' by zero.",
"Cannot divide type 'Literal[2]' by zero.",
"Cannot divide type 'Literal[3]' by zero.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modulo requires division to calculate, so I think the error message is accurate and adequate as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants