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

implement field visibility #345

Merged
merged 4 commits into from
Jan 10, 2025
Merged

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Jan 9, 2025

Code is a bit ugly, can be cleaned up if necessary

Currently checks for the line info of the field access to compare modules. An alternative might be to track the current instantiated routine symbol in a stack which would include templates

Imported modules don't seem to compile to C yet so had to disable testing the working cases

The type conversion in:

template getPrivateTempl*[T](x: Generic[T]): T =
  T(x.private)

is a workaround, the type of x.private is the T in the original definition of Generic. This only errors for templates because other routines use commonType since #320, which matches anything to typevar expected types. Maybe x.private can substitute the type of private according to the InvokeT params of the type of x, this would have to be done for inherited types too.

@metagn metagn marked this pull request as ready for review January 10, 2025 01:27
withNewScope c:
# copy toplevel scope status for exported fields
Copy link
Member

Choose a reason for hiding this comment

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

Wait ... what? Why?

Copy link
Collaborator Author

@metagn metagn Jan 10, 2025

Choose a reason for hiding this comment

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

Exporting a symbol is only allowed if the current scope has kind ToplevelScope, so this is propagated from object declarations in toplevel scopes to the scope of their fields so that the fields can be exported. The only other use of scope kinds seems to be for whether or not to make global or local syms in identToSym which already unconditionally makes global syms for field symbols, so I thought it was sufficient to change it.

Generics in general also open a new scope though (the other place where this is done) which is a bit more murky, this is supposed to work:

type Foo[T] = object
  field*: T

But this would compile even though it doesn't add the symbols to the top level scope:

type Foo[T] = (; const field* = 123; T)

Maybe a ToplevelFieldScope kind instead?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but can be done later.

@Araq
Copy link
Member

Araq commented Jan 10, 2025

Code is a bit ugly, can be cleaned up if necessary

We rarely if ever find the time to clean up things so it's better to clean it up right now. That said, I don't find it particularly ugly.

@Araq Araq merged commit 36c7a43 into nim-lang:master Jan 10, 2025
3 checks passed
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