Implement ReadOnlyReason for specific error messages #570

Adist319 · 2025-06-25T01:08:09Z

This PR completes the architectural refactoring to provide more descriptive errors for assignments to read-only fields, addressing issue #521. The core change replaces the simple readonly: bool flag with a more expressive readonly: Option type across the checker.

…actor more read_only fields from bool to the new enum

- Centralizes read-only logic. - Updates usage sites to fix compilation errors. - Fixes readonly bug with methods in frozen dataclasses. - Removed unused read_only functions

…g frozen dataclasses, named tuples, and read-only annotations. Updated test cases to reflect the new error messages for better clarity. Deferred implementation for Final fields until further context work is completed. + formatting

…c error messages based on ReadOnlyReason. This was accidentally overriden when resolving merge conflicts after rebasing

…into read_only_reason

Adist319 · 2025-06-25T05:19:20Z

Just a note: I ran python test.py on a Windows environment, which gave me a conformance output with Windows-style backslashes. Let me know if this is expected.

Other than that, the expected error messages for several tests have been updated from the generic Cannot assign to read-only attribute... to the new, more specific messages, for example:
Cannot assign to field 'x'; it is part of a frozen dataclass
Cannot assign to field 'x'; it is part of a NamedTuple

And we could address #521 (comment) in a separate issue/PR.

yangdanny97 · 2025-06-25T14:12:43Z

pyrefly/lib/alt/class/class_field.rs

+    ) -> Option<ReadOnlyReason> {
+        // 1. Check annotations first (highest precedence)
+        if let Some(ann) = annotation {
+            // if ann.is_final() {


why's this commented out?

I commented this out because we're deferring the Final + init logic to a separate PR, but let me know if you want this removed instead.

We can leave the existing behavior of marking Final attrs that are initialized on the class as read-only, but leave Final attrs that are initialized in the constructor alone.

Yup, thanks for the catch, I believe the existing behavior should already be back in the latest changes.

yangdanny97 · 2025-06-25T14:13:22Z

pyrefly/lib/alt/class/class_field.rs

-            Attribute::read_only(make_bound_method(instance, attr).into_inner())
+        attr if is_class_var => Attribute::read_only(
+            make_bound_method(instance, attr).into_inner(),
+            ReadOnlyReason::Inherited,


I think the guard is saying, this attribute is a ClassVar, so it can't be set from the instance

see https://typing.python.org/en/latest/spec/class-compat.html#classvar

yangdanny97 · 2025-06-25T14:14:34Z

pyrefly/lib/alt/class/class_field.rs

@@ -109,7 +110,7 @@ enum ClassFieldInner {
        ty: Type,
        annotation: Option<Annotation>,
        initialization: ClassFieldInitialization,
-        readonly: bool,
+        readonly: Option<ReadOnlyReason>,


Nit: could you add a comment here explaining that None means it's read-write?

Also, it might make sense to rename to readonly_reason

Was actually debating on renaming to readonly_reason, but didn't get around to it. Will do!

yangdanny97 · 2025-06-25T14:15:35Z

pyrefly/lib/alt/solve.rs

@@ -1733,7 +1733,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
            (Type::TypedDict(typed_dict), Type::Literal(Lit::Str(field_name))) => {
                let field_name = Name::new(field_name);
                if let Some(field) = self.typed_dict_field(typed_dict, &field_name) {
-                    if field.read_only {
+                    if field.read_only.is_some() {


Would prefer a is_read_only() helper method to get the boolean value, to avoid exposing the implementation unnecessarily. The read_only field can remain public, but maybe rename to readonly_reason as well.

yangdanny97 · 2025-06-25T14:17:14Z

pyrefly/lib/solver/subset.rs

@@ -730,13 +730,13 @@ impl<'a, Ans: LookupAnswer> Subset<'a, Ans> {

                want_fields.iter().all(|(k, want_v)| {
                    got_fields.get(k).is_some_and(|got_v| {
-                        match (got_v.read_only, want_v.read_only) {


After you address https://github.com/facebook/pyrefly/pull/570/files#r2166837276, these Some/None checks can go back to being true/false checks

yangdanny97 · 2025-06-25T14:19:37Z

pyrefly/lib/alt/class/class_field.rs

-        });
-
-        // Read-onlyness
-        let readonly = is_namedtuple_member


the logic here re: final attrs initialized on the class doesn't seem to be preserved in determine_readonly_reason

Good catch. Will add this logic back.

yangdanny97 · 2025-06-25T14:20:51Z

pyrefly/lib/alt/class/class_field.rs

+        let metadata = solver.get_metadata_for_class(cls);
+
+        if metadata.named_tuple_metadata().is_some() {
+            return Some(ReadOnlyReason::NamedTupleField);


NamedTuples can have attributes that are not elements. The spec says:

A named tuple class can be subclassed, but any fields added by the subclass are not considered part of the named tuple type.

I think the added fields should not be marked as read-only

The previous logic handled this, so it can probably be copied over.

Got it, didn't actually know this about NamedTuples.

yangdanny97

Thanks! Back to you with some comments.

DW about the line endings, they'll be automatically regenerated when this is merged. If it's causing a lot of noise I can look into it.

…only_reason` across multiple files. Introduced `is_read_only` method for better readability and consistency. Updated related logic in `AnswersSolver`, `ClassField`, and `TypedDictField` to utilize the new structure.

Summary: same as below diff but for delattr Reviewed By: stroxler Differential Revision: D76846735 fbshipit-source-id: 496a91f2a01ff7c707f9053b55ef98f3d4a780b2

Adist319 · 2025-06-25T21:14:11Z

Pushed my new changes. Thanks for the detailed feedback @yangdanny97!

Edit: I noticed that a few tests in the suite — like test_mypy_demo, test_module_type, and test_nested_func have been running for over 60 seconds on CI. Just wanted to check if that’s expected behavior or if it might be a hang or a recent regression.

For what it's worth, cargo test --release completes normally on my local machine.

Summary: This diff and the next add functionality that enables users to specify a conda environment name that will be used, even when not activated, unless: - a venv is active - a conda env is active - a venv is present in the project directory This diff queries a `conda` executable for the all of the environments, then determines if one exists and finds its interpreter. Reviewed By: yangdanny97 Differential Revision: D77247851 fbshipit-source-id: a3ef317a45fa916e7a4e5774e2c48f30170173fc

Summary: This diff and the previous add functionality that enables users to specify a conda environment name that will be used, even when not activated, unless: - a venv is active - a conda env is active - a venv is present in the project directory This diff finishes the functionality for using conda environments, even if they're not active. In the `configure` function, we add the ability to query conda for an environment. We also update the LSP way of overriding a config to match what we do when a `python-interpreter` is set, though we may be removing that in the future. Reviewed By: yangdanny97 Differential Revision: D77247852 fbshipit-source-id: 1fefe95d0bd06a0424c2bc7d53c5b2890e4a609d

Summary: This diff reverts D77273848 Causes an infinite loop Depends on D77273848 Reviewed By: rchen152 Differential Revision: D77348553 fbshipit-source-id: 865f66a528b25e69215801df4609b670ecf0edaa

Summary: Make it clear this is what we report to the user. Reviewed By: stroxler Differential Revision: D77299757 fbshipit-source-id: 42042cee418dd0cd6ce84180b0dfe90e3deefb42

Reviewed By: kinto0 Differential Revision: D77299766 fbshipit-source-id: ac65db715154459d0027dbdfaecd96668b14ec45

Summary: Just moving the code around. Reviewed By: kinto0 Differential Revision: D77299745 fbshipit-source-id: d80dbb9c1ac2bd6760eb6c2e2f07771465fb9323

Summary: Only used once, can just do it in terms of contents. Reviewed By: kinto0 Differential Revision: D77299770 fbshipit-source-id: f950b1305e93917a97e65a266356bdf50fbe5002

Summary: Not yet used; we'll call this when we detect a cycle. Reviewed By: ndmitchell Differential Revision: D76948131 fbshipit-source-id: d439ebd750899b1fe1bc91a7c223af179234b336

Summary: We'll need a pre-calculation check for deterministic cycle breaking because we need to know when we are recursing from a cycle detection point toward the break-at point: all the intervening calculations have to bump their per-thread recursion limit. We'll need a post-calculation check for two reasons: - In the simplest version, we'll need to know when we finished the cycle, so we can pop it from the cycle stack. - Eventually, we will also need some extra logic for all the intervening calculations to avoid writing answers that contain unpinned `Var`s (without this, we'll still be exposed to data races). Even thought the uses of the two checks differ, the same enum works pretty well for both, because there are always three cases: - There's no cycle to worry about, just do default things - We're a participant in the cycle but not the break point, do whatever that entails - We're at the breakpoint, do whatever that entails Reviewed By: ndmitchell Differential Revision: D76948141 fbshipit-source-id: 0592d18848453353cbfe0204fa9db38dc86648b6

Summary: We used to only use it for the calculation stack so we created it inside `push`, but we're about to need it for other things. Reviewed By: ndmitchell Differential Revision: D76948128 fbshipit-source-id: 5d9b77a6a111cfee1d7749ce7081c3fbd7440160

Summary: In order to implement deterministic cycle breaks, I'm going to wind up needing a double match, where the exact logic will depend on some combination of the cycle state of the current idx and the calculation state. It's a little hard to see how to do that well without rearranging some of the fiddly logic more clearly - this diff mostly leaves things as they were but moves the fiddling into helpers. Reviewed By: ndmitchell Differential Revision: D76948134 fbshipit-source-id: f533c3323a2724030385e1fa1084003f85fd07a1

Summary: A lot of the complexity in the existing logic came from making every branch of a match return a type complicated enough to control another match because we were trying to calculate or fetch the result and then record recursive correspondence independently. But these two operations aren't actually independent - the only scenario where we would record recursive is when we've just stored an answer. Inlining that makes things a bit simpler, the types make more sense and we get rid of one of the blocks in `get_idx`. Reviewed By: ndmitchell Differential Revision: D76948142 fbshipit-source-id: 34fe1a0082805a151ed0adec5035bc79a946a6d0

Summary: Eliminate another unnecessary result type and match; now the body of `get_idx` is mostly just one big match. This is a big help since I need to figure out how to add some complexity while keeping it readable. Reviewed By: ndmitchell Differential Revision: D76948135 fbshipit-source-id: ec8dac7c1cef41cd4950e06dfaa669b7fd0dc38d

Summary: Trying to keep the upcoming diff where I implement cycle detection as clean as possible; I realized a couple methods are cleaner if we can pass `CalcId` by reference, and we still need an api to pop the cycle when we finish it. Reviewed By: ndmitchell Differential Revision: D76948133 fbshipit-source-id: f2e42dcf3b52e441ca8ad47192274a47d519a02b

Summary: Otherwise windows users generate them with `\r\n` Reviewed By: ndmitchell Differential Revision: D77378036 fbshipit-source-id: f53556dafc61dedb038762ef8fe1f7ab19084551

Summary: reported [here](https://discord.com/channels/1319086885024567347/1387701966527795200/1387814602158506124) we might also want to allow auto-imports of builtins < 3 characters Reviewed By: SamChou19815 Differential Revision: D77379581 fbshipit-source-id: b808302e6c5fb7124b9a75339fcdf7956a217647

Reviewed By: grievejia, yangdanny97 Differential Revision: D77389555 fbshipit-source-id: 0d9b7ee311d0db4c2482a3f38d18769adae66479

Summary: Pull Request resolved: facebook#576 This code finds module prefixes in the order they appear on disk. Often files written in one order will read in that order, but there is no guarantee, and it's file system dependent. To make sure these tests are deterministic on all file systems, sort the result. Note that before the result is used in LSP we sort it anyway, so it doesn't need sorting in the real method. Reviewed By: grievejia Differential Revision: D77373942 fbshipit-source-id: fc333f02c1c4f48aaef81c3032ae5be051f8936b

Summary: Nicer API. Reviewed By: SamChou19815 Differential Revision: D77389554 fbshipit-source-id: ca9cfa021d03b6b5fdf3146ac3f7d71601165fcc

Summary: We always make this `Var` or `()`. So might as well simplify and avoid a lot of boilerplate per key type. Reviewed By: rchen152 Differential Revision: D77391369 fbshipit-source-id: e3f8b8064ceb5fa64e47953f60ce9e24a5882bff

Summary: Just an alias for Keyed. Reviewed By: rchen152 Differential Revision: D77391368 fbshipit-source-id: 76173bc48de5a53999e4013939ad1026ad8a90d0

…actor more read_only fields from bool to the new enum

- Centralizes read-only logic. - Updates usage sites to fix compilation errors. - Fixes readonly bug with methods in frozen dataclasses. - Removed unused read_only functions

…g frozen dataclasses, named tuples, and read-only annotations. Updated test cases to reflect the new error messages for better clarity. Deferred implementation for Final fields until further context work is completed. + formatting

…only_reason` across multiple files. Introduced `is_read_only` method for better readability and consistency. Updated related logic in `AnswersSolver`, `ClassField`, and `TypedDictField` to utilize the new structure.

…into read_only_reason

Adist319 · 2025-06-26T20:57:31Z

Will get back to this, the rebase turned out messier than expected and I likely messed something up.

Edit: I’ll probably open a new branch and PR to keep the commit history clean.

rchen152

Review automatically exported from Phabricator review in Meta.

yangdanny97 · 2025-06-27T02:12:14Z

@Adist319 no worries, I can rebase and clean it up for you. Thanks for your work on this!

facebook-github-bot · 2025-06-27T03:30:41Z

@yangdanny97 merged this pull request in d186028.

Summary: This PR completes the architectural refactoring to provide more descriptive errors for assignments to read-only fields, addressing issue facebook#521. The core change replaces the simple readonly: bool flag with a more expressive readonly: Option<ReadOnlyReason> type across the checker. Pull Request resolved: facebook#570 Test Plan: test.py Rollback Plan: Reviewed By: rchen152 Differential Revision: D77376730 Pulled By: yangdanny97 fbshipit-source-id: 2878b38db1999379210f628bc60bf0ed46b65331

Adist319 added 3 commits June 24, 2025 17:28

Added ReadOnly enum, setting up initial ground work, will need to ref…

58d2e74

…actor more read_only fields from bool to the new enum

refactor: Replace readonly bool with ReadOnlyReason enum

1f5be7d

- Centralizes read-only logic. - Updates usage sites to fix compilation errors. - Fixes readonly bug with methods in frozen dataclasses. - Removed unused read_only functions

facebook-github-bot added the cla signed label Jun 25, 2025

Adist319 and others added 5 commits June 24, 2025 18:11

format check, accidentally undid in previous commit

4257128

Enhance error handling for read-only attributes by generating specifi…

c4bc64c

…c error messages based on ReadOnlyReason. This was accidentally overriden when resolving merge conflicts after rebasing

Merge branch 'main' into read_only_reason

0ed2afd

Format check

6e1d2d7

Merge branch 'read_only_reason' of https://github.com/Adist319/pyrefly …

9d788a2

…into read_only_reason

yangdanny97 reviewed Jun 25, 2025

View reviewed changes

yangdanny97 requested changes Jun 25, 2025

View reviewed changes

Adist319 and others added 4 commits June 25, 2025 14:00

Added conformance diffs, and format checks

fc13668

Merge branch 'main' into read_only_reason

bb494bf

fall back to __delattr__ for attribute deletion

ee76237

Summary: same as below diff but for delattr Reviewed By: stroxler Differential Revision: D76846735 fbshipit-source-id: 496a91f2a01ff7c707f9053b55ef98f3d4a780b2

connernilsen and others added 7 commits June 25, 2025 14:37

Revert D77273848

6b04909

Summary: This diff reverts D77273848 Causes an infinite loop Depends on D77273848 Reviewed By: rchen152 Differential Revision: D77348553 fbshipit-source-id: 865f66a528b25e69215801df4609b670ecf0edaa

Rename SourceRange to UserRange

4b518d8

Summary: Make it clear this is what we report to the user. Reviewed By: stroxler Differential Revision: D77299757 fbshipit-source-id: 42042cee418dd0cd6ce84180b0dfe90e3deefb42

Rename associated SourceRange functions

6dd6abc

Reviewed By: kinto0 Differential Revision: D77299766 fbshipit-source-id: ac65db715154459d0027dbdfaecd96668b14ec45

Push a lot of LinedBuffer operations down to the type

387c4c2

Summary: Just moving the code around. Reviewed By: kinto0 Differential Revision: D77299745 fbshipit-source-id: d80dbb9c1ac2bd6760eb6c2e2f07771465fb9323

Delete ModuleInfo.len

6e8f6ef

Summary: Only used once, can just do it in terms of contents. Reviewed By: kinto0 Differential Revision: D77299770 fbshipit-source-id: f950b1305e93917a97e65a266356bdf50fbe5002

stroxler and others added 22 commits June 26, 2025 08:58

Add a cycle_detected method to Cycles

8fe106c

Summary: Not yet used; we'll call this when we detect a cycle. Reviewed By: ndmitchell Differential Revision: D76948131 fbshipit-source-id: d439ebd750899b1fe1bc91a7c223af179234b336

Compute current in get_idx

c98ff6d

Summary: We used to only use it for the calculation stack so we created it inside `push`, but we're about to need it for other things. Reviewed By: ndmitchell Differential Revision: D76948128 fbshipit-source-id: 5d9b77a6a111cfee1d7749ce7081c3fbd7440160

specify newlines for conformance output

e668d75

Summary: Otherwise windows users generate them with `\r\n` Reviewed By: ndmitchell Differential Revision: D77378036 fbshipit-source-id: f53556dafc61dedb038762ef8fe1f7ab19084551

Fix a lint

b55feb5

Reviewed By: grievejia, yangdanny97 Differential Revision: D77389555 fbshipit-source-id: 0d9b7ee311d0db4c2482a3f38d18769adae66479

Make Unique/Var zero's with const

dc11b59

Summary: Nicer API. Reviewed By: SamChou19815 Differential Revision: D77389554 fbshipit-source-id: ca9cfa021d03b6b5fdf3146ac3f7d71601165fcc

Hardcode ::Recursive to Var

c72191a

Summary: We always make this `Var` or `()`. So might as well simplify and avoid a lot of boilerplate per key type. Reviewed By: rchen152 Differential Revision: D77391369 fbshipit-source-id: e3f8b8064ceb5fa64e47953f60ce9e24a5882bff

Delete SolveRecursive

ab61fa2

Summary: Just an alias for Keyed. Reviewed By: rchen152 Differential Revision: D77391368 fbshipit-source-id: 76173bc48de5a53999e4013939ad1026ad8a90d0

Added ReadOnly enum, setting up initial ground work, will need to ref…

e17a50b

…actor more read_only fields from bool to the new enum

refactor: Replace readonly bool with ReadOnlyReason enum

0a476be

- Centralizes read-only logic. - Updates usage sites to fix compilation errors. - Fixes readonly bug with methods in frozen dataclasses. - Removed unused read_only functions

format check, accidentally undid in previous commit

84c9f5c

Format check

98d6761

Added conformance diffs, and format checks

f959eb6

Merge branch 'read_only_reason' of https://github.com/Adist319/pyrefly …

1711c72

…into read_only_reason

rchen152 approved these changes Jun 26, 2025

View reviewed changes

facebook-github-bot closed this in d186028 Jun 27, 2025

facebook-github-bot added the Merged label Jun 27, 2025

Implement ReadOnlyReason for specific error messages #570

Implement ReadOnlyReason for specific error messages #570

Conversation

Adist319 commented Jun 25, 2025

Uh oh!

Adist319 commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangdanny97 Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangdanny97 Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangdanny97 Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangdanny97 left a comment

Choose a reason for hiding this comment

Uh oh!

Adist319 commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adist319 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchen152 left a comment

Choose a reason for hiding this comment

Uh oh!

yangdanny97 commented Jun 27, 2025

Uh oh!

facebook-github-bot commented Jun 27, 2025

Uh oh!

Uh oh!

Adist319 commented Jun 25, 2025 •

edited

Loading

yangdanny97 Jun 25, 2025 •

edited

Loading

yangdanny97 Jun 25, 2025 •

edited

Loading

yangdanny97 Jun 25, 2025 •

edited

Loading

Adist319 commented Jun 25, 2025 •

edited

Loading

Adist319 commented Jun 26, 2025 •

edited

Loading