Skip to content
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

Update to Pyre-0.9.21 #1734

Merged
merged 2 commits into from
May 13, 2024
Merged

Update to Pyre-0.9.21 #1734

merged 2 commits into from
May 13, 2024

Conversation

stroxler
Copy link
Contributor

I followed the instructions in CONTRIBUTING:

  • set up a Python 3.12.3 virtual environment
  • pip install -r the requirements
  • run python3 main.py

Changes to Pyre reflected here:

  • implemented support for assert_type, which
    • causes us to pass the assert_type test
    • fixes a lot of false positives on other tests, as well as a few false negatives
      where Pyre is actually not conformant and our lack of assert_type support was hiding it
  • fixed typing.reveal_type support
  • improved dataclasses support; there are still gaps but we pass more of the tests
  • miscellaneous changes (both bugfixes and improvements) not directly motivated by conformance

Open questions for reviewers:

  • I presumably need to update some manual fields
    • the conformant field in some places where the automated conformance results changed
    • the notes in some places
    • ... is this all I need to do manually?
  • the tests for reveal_type are auto-marked as failing but I think Pyre is outputting as expected
    here, should I mark it as passing (or partial) in the conformant field?

@JelleZijlstra
Copy link
Member

I presumably need to update some manual fields

Yes, the "Conformant" and "Notes" fields should be updated. Ideally "Conformant" should mirror the conclusion from the automated check, but there may be some remaining cases where we need a manual override.

  • the tests for reveal_type are auto-marked as failing but I think Pyre is outputting as expected
    here, should I mark it as passing (or partial) in the conformant field?

I think the problem here is that pyre outputs "errors" for reveal_type(), while pyright and mypy output some lower-severity diagnostic. Either decision is fine and acceptable but the tests currently are written assuming the second option. I'll have to think of a way to represent this in the automatic conformance scoring, but for now I think you should mark Pyre as passing for this test.

@erictraut
Copy link
Collaborator

Thanks @stroxler, this is great progress!

After looking through all of the changes, I think the following manual scoring updates should be made:

annotations_coroutines: Conformance should be changed from "Partial" to "Pass" and the "notes" removed.
annotations_typeexpr: Conformance should be changed from "Partial" to "Pass" and the "notes" removed.
dataclasses_order: Conformance should be changed from "Partial" to "Pass" and the "notes" removed.
directives_assert_type: Conformance should be changed from "Partial" to "Pass" and the "notes" removed.
specialtypes_none: Conformance should be changed from "Partial" to "Pass" and the "notes" removed.

directives_reveal_type: Conformance should be changed from "Unsupported" to "Pass"; "notes" should perhaps explain that error is generated rather than some lower-severity diagnostic.

exceptions_context_managers: Conformance should be changed from "Pass" to "Partial" and notes added about failures.
literals_literalstring: Conformance should be changed from "Pass" to "Partial" and notes added about failure.

@stroxler
Copy link
Contributor Author

stroxler commented May 12, 2024

Thanks!

There are a lot of changes here and I'm trying to do a quick pass through every module, in particular updating the notes where we need to (quite a few of the previous notes were due to us not having assert_type support which is fixed) so that it's easier for the Pyre team to see our gaps.

I'll double check the ones you flagged @erictraut; I think for exceptions_context_managers I'll actually mark it as "Unsupported" because I'm pretty sure we 100% fail this (the lack of assert_type support made it look like we did).

I think I might mark reveal_type as partial for now, since it probably would be better to use a different diagnostic if that's the norm. I'll check with the rest of the team on Monday before making a final call on that one.

Copy link

cpython-cla-bot bot commented May 13, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@stroxler stroxler changed the title Autogenerate results on Pyre-0.9.21 Update to Pyre-0.9.21 May 13, 2024
Edit the notes where there were obvious updates, particularly related
to changes because of `assert_type` being supported.

Mark a few tests as Passing that were not before, and some others as
either Unsupported or Partial where they were previously marked as
better-supported (many of these are again related to `assert_type`;
improved support for that makes it clearer that in a few cases Pyre was
not conformant but appeared to be due to not throwing errors).
@stroxler
Copy link
Contributor Author

stroxler commented May 13, 2024

I've finished my manual adjustments.

I updated a few tests beyond the ones @erictraut, and decided that exceptions_context_managers should really be marked as "Unsupported", it's not really even partially working.

I'll ask the team about whether we're okay taking an action item to make reveal_type spit out non-error diagnostics tomorrow.

The main other things I came across:

  • The tests sometimes assume that assert_type erases literal types (Pyre winds up failing a lot of the asserts when it's easy to infer a literal), is that currently part of the spec?

  • Pyre throws a lot of uninitialized attribute errors, I might want to look into making the error diff tool ignore those in most cases (although I'm of two minds on this, because some of these errors are actually wrong). Alternatively, we could update the conformance tests not to trigger these errors by always initializing.

  • There's at least one case (line 61 of tuples_unpacked.py) where the error marking is maybe over-sensitive on line numbers; Pyre complains on the line where an invalid annotation that got split across lines starts, but the tests insist that the error ought to be thrown in a nested sub-expression.

@erictraut
Copy link
Collaborator

erictraut commented May 13, 2024

is [erasure of literal types] currently part of the spec?

The spec for assert_type doesn't specify whether literal types should be erased for purposes of type comparison, but I don't think mypy or pyright do so. Can you provide an example of where pyre's behavior differs from mypy or pyright in this regard? I suspect the issue is not with assert_type but is rather with type inference assumptions in the conformance tests. As you're aware, type inference rules are not covered in the spec, so we need to take care in the tests not to "bake in" any assumptions about type inference. For example:

x = 1
assert_type(x, Literal[1]) # No error in pyright but error in mypy

This is why most of the tests are written in functions where the parameters have explicit type annotations:

def func1(x: int):
    assert_type(x, int)

Alternatively, we could update the conformance tests not to trigger these errors by always initializing

I'd prefer to update the conformance tests not to trigger these errors by always initializing. I tried to do that consistently when writing the tests, but there are apparently some cases that I missed (probably masked at the time due to other issues). Please feel free to update the tests accordingly — or let me know if you'd prefer that I do it. Either is fine with me.

error marking is maybe over-sensitive on line numbers

There is a mechanism that allows for errors to be reported on one of several different lines. You can read about the details in the README.txt. In short, it involves adding multiple # E comments with a common "tag".

@erictraut
Copy link
Collaborator

The changes look good to me. Merging.

@erictraut erictraut merged commit 9f7f400 into python:main May 13, 2024
5 checks passed
@stroxler
Copy link
Contributor Author

Pyre's current behavior is that assert_type requires an exact match. As a result, in Pyre we get

from typing import assert_type, Literal

x = 1
assert_type(x, Literal[1])  # no error
assert_type(x, int)  # error

This leads to error diffs on quite a few tests.

It seems like most of these tests don't use assert_type on literals directly, but they do use cases where Pyre is able to infer a literal type and then propagate it into a generic context. For example, Pyre preserves a literal type here:
https://github.com/python/typing/blob/main/conformance/tests/generics_scoping.py#L14

The function being called is T -> T, and Pyre infers (correctly, since only the identity can have that signature) that this means calling the function on Literal[1] must produce Literal[1], which fails the assert_type(int).

Other examples involved generic classes constructed out of literals... in these cases Pyre will frequently infer the generic to be over a literal type. This isn't necessarily good or user-friendly behavior, it can force them to use type annotations, but I don't think it's spec-violating either.

We could probably make Pyre smart enough to optionally weaken literal types whenever that would make assert_type pass, and/or revisit our generic type inference to default to non-literal types. But in the meantime, Pyre does complain on cases like these.

@erictraut
Copy link
Collaborator

Ah, so this is a constraint solver issue. As with type inference, constraint solver behaviors are not specified in the typing spec. I didn't realize that any type checker retained literals in its constraint solver. Pyright doesn't retain literals in this case unless there is a constraint that requires it. Mypy generally doesn't retain literals, but there are cases where it does. Its behavior is inconsistent.

I'm surprised that pyre retains literals in this case. I'm not convinced this is type safe. I don't think it's true that "only the identity can have that signature". For example:

def func[T: int](x: T) -> T:
    return type(x)(x + 1)

We could probably make Pyre smart enough to optionally weaken literal types whenever that would make assert_type pass...

I don't think that's the right answer. That would defeat the purpose of assert_type. Instead, I think we should come to an agreement about which behaviors are in-spec and out-of-spec and amend the tests accordingly. In cases where the spec allows a variety of behaviors, we'll need to avoid using assert_type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants