sem: all tuple fields now have an owning sym #816
Conversation
an owner. This means all fields in a tuple will have a shared owner, allowing for querying for reflection or code-gen. This required modifying `semAnonTuple` and `semTuple` procedures in the `semtypes` module. Now these procedures check to see if an owning symbol already exists and if not an anonymous one is created.
|
tests seem to pass locally, just doing a quick check here. |
There was a problem hiding this comment.
I've noticed a potential issue with the setupOwner condition, and there might be an expectation violation with assigning the anonymous symbol to the type's owner, but I think that the direction of assigning symbols to anonymous types is good.
It's nothing that has to happen as part of this PR, but using an anonymous symbol would also make sense for anonymous procedural types.
| @@ -500,6 +497,17 @@ proc semAnonTuple(c: PContext, n: PNode, prev: PType): PType = | |||
| if n.len == 0: | |||
| localReport(c.config, n, reportSem rsemTypeExpected) | |||
| result = newOrPrevType(tyTuple, prev, c) | |||
| let setupOwner = c.inGenericContext == 0 and result.sym.isNil | |||
There was a problem hiding this comment.
I haven't verified it, but I think the check is an issue in the following case:
type
Generic[T] = object
f: tuple[a, b: int]When the tuple type is sem-checked, inGenericContext is greater than 0 and result.sym is nil.
There was a problem hiding this comment.
Doh, good call.
I was suspicious of the owner pushing in the right hand side type section analysis for generics, and I think that's coming home to roost here.
| nextSymId c.idgen, c.getCurrOwner(), n.info) | ||
| sym.flags.incl sfAnon | ||
| sym.typ = result | ||
| result.owner = sym |
There was a problem hiding this comment.
If a symbol directly represents the type, then it is generally assigned to the types's sym field instead of owner, which for consistency would also make sense here, I think. (typesrenderer already checks for sfAnon symbols, so this should not cause problems)
There was a problem hiding this comment.
It did, but more so for the silly getTypeDecl and that whole silly API in std/macros. I'll have to dig in further to see what's going wrong there.
|
Thanks for the review, @zerbina |
move `newAnonSym` from the module `semexprs` to `sem`, allowing it to be reused in `semtypes`.
if a discriminant was too large for a variant an error was both not logged nor did it cause compilation to fail
|
the approach I'm taking here isn't quite right. I think more needs to be cleaned up and then a less hacky approach to symbol creation can be taken. |
Summary
All tuple field symbols now have an owning tuple type symbol.
Details
For tuples without names create a anonymous sym as
an owner. This means all fields in a tuple will have a shared owner,
allowing for querying for reflection or code-gen.
This required modifying
semAnonTupleandsemTupleprocedures in thesemtypesmodule. Now these procedures check to see if an owning symbolalready exists and if not an anonymous one is created.
Notes for Reviewers