-
Notifications
You must be signed in to change notification settings - Fork 533
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
Evaluate arbitrary markers to false
#3681
Conversation
crates/uv-resolver/src/marker.rs
Outdated
@@ -35,7 +35,8 @@ pub(crate) fn is_disjoint(first: &MarkerTree, second: &MarkerTree) -> bool { | |||
string_is_disjoint(expr1, expr2) | |||
} | |||
MarkerExpression::Extra { operator, name } => extra_is_disjoint(operator, name, expr2), | |||
MarkerExpression::Arbitrary { .. } => false, | |||
// `Arbitrary` expressions always evaluate to `false`, and are thus always disjoint. | |||
MarkerExpression::Arbitrary { .. } => true, |
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.
@ibraheemdev -- is this wrong? I was trying to understand the comment here: #3679 (comment)
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.
This sounds right to me fwiw
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.
Yes, that is correct. However, I wonder if maybe we should be pessimistic regarding forking on meaningless expressions, cc @BurntSushi.
Also if we do make this change, I expected a test to be failing. I think we need to check if expr2
is Arbitrary
as well, I can make that change in a later PR.
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.
Okay, think I fixed this.
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.
I'm not fully sure. My instinct is also that we probably ought to be pessimistic on forking here. If we say two arbitrary expressions are disjoint, then we will fork. But logically speaking, if the arbitrary expressions are always false, then, well, I guess that makes sense?
I'm happy to let practice dictate what we do here. (So this is fine for now.)
MarkerExpression::Arbitrary { .. } => true, | ||
MarkerExpression::Arbitrary { .. } => false, |
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.
If we always evaluate these to false that means that we will skip any requirement that has unknown marker expressions right? It seems like true would ignore the bad marker which makes more sense?
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.
"true and warn that it's meaningless" seems like good behavior?
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.
I believe this was our behavior until very recently (return false
here) and is pip's behavior.
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.
To be explicit: I think we should omit requirements that have invalid markers, not include them.
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.
I agree with reverting back to our original behavior here, and fine with leaving arbitrary expression forking as an open question.
5db9d95
to
4d08aed
Compare
4d08aed
to
18ad21b
Compare
Summary
See: #3679 (comment).
Closes: #3675 (although I think we have another improvement to make there -- will file separately).