-
-
Notifications
You must be signed in to change notification settings - Fork 389
No unknown operations diagnostic #3303
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
base: master
Are you sure you want to change the base?
Conversation
…st function calls
…here both args are known
Summary of ChangesHello @NathanSnail, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new diagnostic, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable new diagnostic, no-unknown-operations, to detect operations that result in an unknown type, which often indicates a typo or a bug. The implementation is solid, adding a new isCallable helper and refactoring operator handling for better clarity and reuse. The changes are well-supported by a comprehensive set of tests. I've added a couple of suggestions to remove TODO comments where the existing code is already robust or the behavior is correct, which will improve code clarity.
| -- TODO: no-unknown doesn't do this but missing-local-export-doc does, is this actually needed? | ||
| if not state.ast then return end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO here questions the necessity of the if not state.ast then return end check. It's a good practice to keep this check for robustness. It acts as a safeguard against potential crashes if files.getState(uri) returns a state object that, for some reason (like a severe parsing error), lacks an AST. Keeping this check makes the diagnostic more resilient. I'd recommend removing the TODO comment.
| -- TODO: it seems that if the operator is defined and the other arg is unkown it is always inferred as the | ||
| -- return type of the operator, so we can't check that case currently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment discusses a scenario where an operation with a known type and an unknown type results in unknown. The current implementation correctly chooses not to report a diagnostic in this case. This diagnostic should focus on operations that are inherently invalid for the given known types, rather than issues stemming from a variable that is already unknown. The no-unknown diagnostic is better suited for flagging the unknown variable itself. Since the current behavior is correct, this TODO can be removed to avoid confusion.
Adds diagnostic to prevent operations which result in an unknown value. Differences with no-unknown diagnostic is that no-unknown is when the type of a binding cannot be inferred whereas this is for the types of expressions who's values are inferred resulting in a value who's type can't be inferred (including values which are never used in a binding). Catches common typo mistakes like
Needs
./tools/build-doc.luaand./tools/locale.luarun.I'm not sure if this diagnostic should be enabled by default, it's kind of similar to need-check-nil and if it does trigger it's almost certainly a bug which is unlike the other disabled by default diagnostics which are more code quality focused.