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

TypeNode#instance_vars should error when instance vars are not setup #10662

Open
straight-shoota opened this issue Apr 28, 2021 · 3 comments · May be fixed by #15293
Open

TypeNode#instance_vars should error when instance vars are not setup #10662

straight-shoota opened this issue Apr 28, 2021 · 3 comments · May be fixed by #15293
Labels
good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. help wanted This issue is generally accepted and needs someone to pick it up kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:feature topic:compiler:semantic topic:lang:macros

Comments

@straight-shoota
Copy link
Member

In the compiler's processing, instance vars are only initialized after all top-level code has been analyzed. Thus accessing the instance vars in top-level code has no result.

class Foo
  @foo = "foo"
end
{% p! Foo.instance_vars %} # => []

This is unexpected and should be considered a bug. TypeNode#instance_vars should rather error if called before it has a meaningful value.

Original discussion: https://forum.crystal-lang.org/t/unions-and-macro-methods/3243/7

@crysbot
Copy link

crysbot commented Feb 13, 2024

This issue has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/typenode-instance-vars-return-empty-array/6569/4

@straight-shoota straight-shoota added good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. help wanted This issue is generally accepted and needs someone to pick it up labels Feb 13, 2024
@straight-shoota
Copy link
Member Author

This also applies to TypeNode#has_inner_pointers? (new in #14847)

@straight-shoota
Copy link
Member Author

straight-shoota commented Dec 19, 2024

Scrap this comment. Every type of course has a reference to Program 🤦

Implementing this unfortunately isn't trivial because the macro method implementation is not aware of the current compiler stage. A central point such as the Program instance isn't reachable from there.

As an alternative, we could use a flag on each type that tells whether (its) instance variables have been initialized. I'd like to avoid increasing the size by adding another instance variable, though.

Looking at the layout of InstanceVarContainer types, I noticed that @instance_vars is lazily initialized. Maybe we could use its nil state as a signal that instance variables have not yet been initialized?
In fact, almost all types have @instance_vars initialized after the top-level semantic has run. In some cases the collection is empty, but still initialized. I presume that's because somewhere we're calling an accessor to check whether an instance variable exist (Type#lookup_instance_var or something like that) and the accessor initializes it as empty.
The only exceptions are module types (NonGenericModuleType and GenericModuleType): When the module itself doesn't declare any instance variables, @instance_vars is still uninitialized.

I think it could work that we use nil as a signal if we replace all nil instances with an empty collection after top -level semantic has run. We just have to force initializing all @instance_vars to a non-nil value at this point.
@instance_vars should never be mutated again afterwards, so we can even use a singleton for empty values to avoid allocations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. help wanted This issue is generally accepted and needs someone to pick it up kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:feature topic:compiler:semantic topic:lang:macros
Projects
None yet
2 participants