-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Java: exclude writeReplace
-defining classes from Serializable
check
#18415
Java: exclude writeReplace
-defining classes from Serializable
check
#18415
Conversation
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.
Copilot reviewed 2 out of 5 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- java/ql/src/Likely Bugs/Serialization/MissingVoidConstructorsOnSerializable.ql: Language not supported
- java/ql/test/query-tests/MissingVoidConstructorsOnSerializable/MissingVoidConstructorsOnSerializable.expected: Language not supported
- java/ql/test/query-tests/MissingVoidConstructorsOnSerializable/MissingVoidConstructorsOnSerializable.qlref: Language not supported
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
java/ql/test/query-tests/MissingVoidConstructorsOnSerializable/Test.java
Outdated
Show resolved
Hide resolved
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.
LGTM, modulo the typo
@igfoo fixed; please re-approve |
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.
Proxying previous approval
#18371 notes that it is a common pattern to define
writeReplace
and thereby avoid needing to meet the default algorithm's requirements ofSerializable
-tagged classes.This is imprecise since in general the defining class might show up in an object stream anyhow (old version; writeReplace that sometimes keeps the original object or another of the same type; etc), but the query is of low importance anyhow so I went for a low-effort fix. This should simply reduce noise for people using the security-and-quality suite.