Disallow and suppress unsafe assignment#19736
Conversation
Summary of ChangesHello @gundermanc, 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 significantly improves the codebase's type safety by introducing a new ESLint rule that disallows unsafe assignments. The primary goal is to prevent potential runtime errors caused by incorrect type assumptions. To facilitate this change, all current violations of the new rule have been temporarily suppressed, allowing for a focused effort on addressing these issues systematically in future work. Additionally, several areas of the code have been refactored to use more explicit and safer type handling patterns. Highlights
Changelog
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
|
# Conflicts: # packages/core/src/code_assist/converter.ts # packages/core/src/code_assist/experiments/experiments.ts # packages/core/src/code_assist/oauth-credential-storage.ts
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step towards improving the codebase's type safety by enabling the @typescript-eslint/no-unsafe-assignment rule. The approach of adding suppression comments for existing violations as a baseline, with a follow-up issue for cleanup, is a solid strategy for incrementally improving a large codebase. I've noticed that in a few places, you've already gone ahead and fixed the underlying issue instead of just suppressing it, which is excellent! My review comments focus on JSON.parse calls where the data source is external and not guaranteed to be valid, and I've also suggested a more modern alternative for deep cloning objects.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| const loadedMetadata = JSON.parse(jsonData); |
There was a problem hiding this comment.
Assigning the result of JSON.parse to loadedMetadata without runtime validation is unsafe, as the data from GCS might not conform to the expected shape. This could lead to runtime errors when properties like _contextId are accessed later. Since Zod is already used in the project, I recommend defining a Zod schema for the metadata and using schema.parse() or schema.safeParse() to validate the data after parsing. This will ensure type safety and prevent potential crashes.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| const issues: Issue[] = JSON.parse(stdout); |
There was a problem hiding this comment.
Directly casting the result of JSON.parse(stdout) to Issue[] is unsafe. The output from the gh command is not guaranteed to match the Issue interface, which could lead to runtime errors. It would be safer to parse this into an unknown type and then validate its structure, for instance using a Zod schema, before casting it to Issue[].
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| const newSettings = JSON.parse(JSON.stringify(pendingSettings)); |
There was a problem hiding this comment.
Instead of using JSON.parse(JSON.stringify(pendingSettings)) for deep cloning, consider using structuredClone(pendingSettings). It's a modern, more performant, and safer way to deep-clone objects, and it's supported since you're on Node.js >= 20.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | |
| const newSettings = JSON.parse(JSON.stringify(pendingSettings)); | |
| const newSettings = structuredClone(pendingSettings); |
|
Size Change: +625 B (0%) Total Size: 25.2 MB ℹ️ View Unchanged
|
3 participants
Summary
Disallows unsafe assignments, like assignment of
anytyped expressions. This prevents a common type safety issue where code can be written that is declared as one type, but in practice, is another.For now, just suppress all instances.
Fixes: #19719
Additional cleanup tracked under: #19731
Pre-Merge Checklist