Skip to content

Implement a basic model for functools.total_ordering #537

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

fangyi-zhou
Copy link

@fangyi-zhou fangyi-zhou commented Jun 20, 2025

This PR adds modelling of functools.total_ordering (closes #491), which allows a class with (at least) one rich comparison method to automatically have all other rich comparison methods.

At a high level, this PR incorporates the following changes:

  1. Capture is_total_ordering in class metadata if the decorator is present.
  2. Synthesize missing rich comparison methods for those decorated classes.
  3. Report an error if the decorator is present but no rich comparison method is present.

I have some open issues that can be fixed either in this PR after receiving some code pointers, or in subsequent PRs.
Open issues (marked in FIXMEs):

  • Error is currently raised on class declaration, ideally it should be on the decorator
  • Type information for synthesized rich comparison methods are not following the comparison method they're synthesizing from: e.g. if __lt__ is synthesized from __gt__, then they should have the same type. Currently we model the synthesizing order, but we are not giving the same type.
  • Classes that have synthetic methods via multiple pathways will not have all the synthetic methods. For example, it's possible to decorate a class with @dataclass and @total_ordering at the same time, and they should have synthesized methods from both decorators.

@yangdanny97
Copy link
Contributor

Error is currently raised on class declaration, ideally it should be on the decorator

For this, instead of using a boolean flag in the class metadata you can have a TotalOrderingMetadata that holds a range instead (or just an Option<TextRange> to keep it simple, with is_total_ordering being a method that checks that it's not None).

Then you can emit the error according to that range.

@yangdanny97
Copy link
Contributor

yangdanny97 commented Jun 20, 2025

Classes that have synthetic methods via multiple pathways will not have all the synthetic methods. For example, it's possible to decorate a class with @dataclass and @total_ordering at the same time, and they should have synthesized methods from both decorators.

We could probably add a merge method to ClassSynthesizedFields and calculate + merge total_ordering separately from the other stuff.

pub struct ClassSynthesizedFields(SmallMap<Name, ClassSynthesizedField>);

@fangyi-zhou
Copy link
Author

Error is currently raised on class declaration, ideally it should be on the decorator

For this, instead of using a boolean flag in the class metadata you can have a TotalOrderingMetadata that holds a range instead (or just an Option<TextRange> to keep it simple, with is_total_ordering being a method that checks that it's not None).

Then you can emit the error according to that range.

Is there a way to get a text range of the decorator in class_metadata_of? I don't seem to find a way to find the text range from the decorator.

@fangyi-zhou fangyi-zhou changed the title Model functools.total_ordering Implement a basic model for functools.total_ordering Jun 22, 2025
@yangdanny97
Copy link
Contributor

BindingClassMetadata doesn't give this information, but we could change its decorator representation from pub decorators: Box<[Idx<Key>]>, to pub decorators: Box<[(Idx<Key>, TextRange)]>, and populate the range when we create the binding.

decorator_list of the parsed node will have the ranges, so maybe we could edit this function to return the range along with the key

self.ensure_and_bind_decorators(mem::take(&mut x.decorator_list), class_object.usage());

@fangyi-zhou
Copy link
Author

Thanks so much for the code pointers, @yangdanny97! Really appreciate your help!

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.

[pyrefly] special case handling for functools.total_ordering decorator
3 participants