Skip to content

Remember attributes as bitflags#471

Open
vinistock wants to merge 1 commit intomainfrom
01-15-remember_attributes_as_bitflags
Open

Remember attributes as bitflags#471
vinistock wants to merge 1 commit intomainfrom
01-15-remember_attributes_as_bitflags

Conversation

@vinistock
Copy link
Member

@vinistock vinistock commented Jan 15, 2026

Motivation

This PR is a proposal to change how we handle attributes. Right now, we have distinct definition types for each one. In reality, they are methods and all we're interested in remembering is how they were originally defined.

I think we can simplify our code by quite a bit if we just use bitflags to remember how they were originally defined and reuse the MethodDefinition struct to represent them.

The main goal is also to handle attributes correctly without incurring more memory cost. An attr_accessor call should result in 2 declarations. We currently have only one definition created, which means that our definitions_to_declarations connection fails to model it. Similar issue that we discovered when working on module_function.

I don't believe we should change the definitions_to_declarations map to store vectors as that will change the size of the values from 2 bytes to vector size + 2 bytes.

Another added benefit, although a lot less important, is that by handling attributes as methods we might also be able to correctly handle the receiver. For example, in a case like this:

Foo.attr_reader(:baz)

We'd just model it exactly like a method receiver and then in resolution we decide if we go into the singleton level or not based on whether it's a method or an attribute.

Implementation

I added all attribute types as flags, removed all attribute definitions and fixed all of the code I could find.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vinistock vinistock self-assigned this Jan 15, 2026
@vinistock vinistock marked this pull request as ready for review January 15, 2026 20:51
@vinistock vinistock requested a review from a team as a code owner January 15, 2026 20:51
@vinistock vinistock force-pushed the 01-15-remember_attributes_as_bitflags branch 3 times, most recently from d2bbdda to 78f9340 Compare January 19, 2026 18:26
Copy link
Contributor

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

I don't think this is the right direction, we're trying to work around a huge limitation in the design.

We discovered that we need to be able to encode the fact one definition can lead to multiple declaration: module_function, attr_accessor, attr, and most likely a lot of DSLs (AASM, AcvtiveRecord, StateMachine, etc.).

This invalidate the N:1 definitions:declaration principle we though would work and I think we need to address the fundamental issue rather than introducing hacks.

@Morriar
Copy link
Contributor

Morriar commented Jan 19, 2026

Following #485, it also looks like this:

Foo = Struct.new("Bar")

Creates 2 declarations: the struct under Struct::Bar and a constant alias Foo::Bar = Struct::Bar?

@Morriar Morriar mentioned this pull request Jan 19, 2026
vinistock added a commit that referenced this pull request Jan 19, 2026
Closes #128

I added a test to verify that all fully qualified names we generate are
what we expect. All of them were already correct, with two exceptions:

- Attributes. We're not generating the methods related to attributes yet
(proposal in #471)
- Global variables. This is fixed in this PR and there's a slight
caveat. I changed the fully qualified to be just the global variable
name, but I kept the owner as `Object`. I believe this is fine for now,
but we may want to revisit later. Global variables are accessible
everywhere, so maybe for completion/go to definition algorithms, it will
make more sense to place it in `BasicObject`? Or perhaps we need the
magic `<main>`? I'll leave that to a dedicated PR
@vinistock vinistock force-pushed the 01-15-remember_attributes_as_bitflags branch from 78f9340 to 6fac261 Compare January 19, 2026 20:08
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.

2 participants