Skip to content

Conversation

@jjonescz
Copy link
Member

@jjonescz jjonescz commented Sep 13, 2025

When re-inferring a binary operator in nullable walker, we previously replaced the original containing type with a new one if there was any implicit conversion between them, so e.g. operator==(ROS, ROS) would be re-inferred as a member of Span because there is a conversion from Span to ROS. But that doesn't mean the operator can be found on Span! Instead we should only allow conversions that make sense so the operator can be actually found on the re-inferred parent, e.g., a derived relationship (implicit reference conversion).

Fixes #80255.

@jjonescz jjonescz marked this pull request as ready for review September 15, 2025 09:31
@jjonescz jjonescz requested a review from a team as a code owner September 15, 2025 09:31
@AlekseyTs
Copy link
Contributor

I find the PR title "Narrow conversion kinds considered when reinferring methods by nullable walker" not very specific, I might even say confusing:

  1. It looks like modified method affects only binary operators, not any methods
  2. It is not clear what conversion specifically. Between which types, etc.

The description also doesn't clearly describe the context for the change, in my opinion. Consider adjusting. At least the title

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

Apart from title and description, LGTM (commit 1)

@jjonescz
Copy link
Member Author

1. It looks like modified method affects only binary operators, not any methods

I think it does affect only binary operators though - the modified code is inside ReInferBinaryOperator.

2. It is not clear what conversion specifically. Between which types, etc.

I can update the title to say between derived/base types I guess. Thanks.

The description also doesn't clearly describe the context for the change, in my opinion. Consider adjusting.

I'm not sure what you mean, can you elaborate?

@jjonescz jjonescz changed the title Narrow conversion kinds considered when reinferring methods by nullable walker Consider only base/derived type conversion kinds when reinferring methods by nullable walker Sep 16, 2025
@jjonescz jjonescz requested review from a team and RikkiGibson September 16, 2025 07:23
@jjonescz
Copy link
Member Author

  1. It looks like modified method affects only binary operators, not any methods

I think it does affect only binary operators though - the modified code is inside ReInferBinaryOperator.

Ah, that's what you are saying. Sorry, I misread it. I will update the title.

@jjonescz jjonescz changed the title Consider only base/derived type conversion kinds when reinferring methods by nullable walker Consider only base/derived type conversion kinds when re-inferring binary operators by nullable walker Sep 16, 2025
@jjonescz jjonescz changed the title Consider only base/derived type conversion kinds when re-inferring binary operators by nullable walker Improve a check for inheritance relationship between types during binary operator re-inference in nullable walker Sep 16, 2025
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.

Assert fails in NullableWalker.AsMemberOfType

4 participants