Skip to content

Conversation

@yoff
Copy link
Contributor

@yoff yoff commented Nov 25, 2025

When a field is assigned a safe type in a constructor, that field is not exposed.

Copilot AI review requested due to automatic review settings November 25, 2025 14:52
@yoff yoff requested a review from a team as a code owner November 25, 2025 14:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances Java thread safety analysis to recognize when fields are initialized with thread-safe types inside constructors, not just in field initializer expressions. This reduces false positives when fields are assigned thread-safe collections (like ConcurrentHashMap or synchronized maps) in constructors.

Key Changes:

  • Added initialValue predicate to check both field initializers and constructor assignments
  • Modified ExposedField class to use the new predicate for more comprehensive thread-safety checking
  • Added comprehensive test coverage for the new functionality

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
java/ql/lib/semmle/code/java/ConflictingAccess.qll Implements the core logic by adding initialValue predicate and updating ExposedField to check constructor assignments
java/ql/test/query-tests/ThreadSafe/examples/ThreadSafeInitializers.java Adds test cases demonstrating thread-safe initialization patterns in constructors
java/ql/test/query-tests/ThreadSafe/ThreadSafe.expected Updates expected test results to include the new test case alert
java/ql/src/change-notes/2025-11-25-thread-safe-initializers.md Documents the enhancement in the release notes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


/**
* Gets the initial value for the field `f`.
* This is either a static initializer or an assignment in a constructor.
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The documentation says "This is either a static initializer or an assignment in a constructor" but should say "field initializer" instead of "static initializer". A static initializer is a method (<clinit>), whereas this predicate returns the field's initializer expression.

Suggested change
* This is either a static initializer or an assignment in a constructor.
* This is either a field initializer or an assignment in a constructor.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree here. Let's fix the qldoc.

@yoff yoff force-pushed the java/more-thread-safe-initialisers branch from 52296d4 to efb6a79 Compare November 25, 2025 17:14
@yoff
Copy link
Contributor Author

yoff commented Dec 8, 2025

The performance run shows something on palatable/lambda both overall and in a stage related to dataflow. However, the changed code is not evaluated until later and not at all during the highlighted stage or indeed query (it is specific to the concurrency queries). I am leaning towards just accepting the performance run...

@aschackmull
Copy link
Contributor

dca looks fairly uneventful to me.

yoff and others added 4 commits December 16, 2025 10:11
Co-authored-by: Raúl Pardo <raul.pardo@protonmail.com>
Whne a fiels is assigned a safe type in a constructor,
that field is not exposed.
@yoff yoff force-pushed the java/more-thread-safe-initialisers branch from a73233c to cbc0100 Compare December 16, 2025 09:11
@yoff
Copy link
Contributor Author

yoff commented Dec 16, 2025

dca looks fairly uneventful to me.

Cool, then I feel ready to merge. I have fixed the QLDoc and rebased (to make CI happy).

@yoff yoff requested a review from aschackmull December 16, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants