-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix(sem): issues with method-call syntax in templates #1298
fix(sem): issues with method-call syntax in templates #1298
Conversation
Instead of going through the `semTemplBody` indirection, directly check whether the name part can be substituted with a template parameter. This also prevents non-gensyms from being bound in the name position.
Using a dedicated branch requires less conditionals, and prepares for changing the dot expression handling. Except for the edge case where a quoted symbol choice (which is nonsense anyway) is inserted into a template body by a macro, the behaviour stays the same.
Move the not-a-qualified-symbol handling into the `if s.isNil` branch.
This lifts the restriction that a gensym'ed symbol cannot be used in a field access position. However, at the moment, it also causes symbols such as `skVar` symbols to be bound there, which is incorrect. Since gensyms don't reach into `semTemplSymbol` anymore, all conditionals including them become false and are thus obsolete.
Only routine symbols and type / generic parameter symbols are considered in field access positions, addressing the problem that symbols of, e.g., locals blocked other symbols. Symbol binding in generics for dot expressions works similar, with the differences that: * types and generic parameters are not considered there * a *closed* symbol choice is forced, instead of an *open* one Using an open symbol choice is wrong (it violates the specification), but is required for backwards compatibility.
It's never incremented and is always zero.
* rename `isField` to the more imperative `noGenSyms` * fix gensyms being added to the symbol choice, even if disabled (the flags of `s` were tested, not those of `a`)
It's now possible that the field part of a dot expression is a identifier coming from a gensym. For example: ```nim template t(x: int) = type Typ {.gensym.} = float x.Typ ``` So that field access still works in this case, `builtinFieldAccess` and `propertyWriteAccess` strip the identifier off of the gensym prefix first.
The `x.T` syntax now works when `T` is a gensym'ed symbos.
The `knownIssue` test is for a bug that's pre-existing.
The field part of a dot expression may now be a symbol choice, which interferes with rendering of custom numeric literals (e.g., `1'lit`), where the special rendering was only enabled for `nkIdent` and `nkSym` nodes. To keep the test working, `getPIdent` (which is used by `isCustomLit`) now also returns the identifier for symbol-choice nodes.
This reverts commit 137c8a7.
Just retrieving the identifier from the symbol choice doesn't work, since then the symbol-choice node is passed to `gident`, where `atom` then crashes because symbol-choices are not allowed (a symbol-choice is, not necessarily, an atom, so this is correct). Instead of `getPIdent` retrieving an identifier from a symbol-choice, `isCustomLit` now special-cases symbol-choices.
This restores the previous behaviour of quoted identifiers with regards to symbol binding of (they ignore injected symbols, for whatever reason).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me a bit. I was approaching the review by looking at the changes, rather than reasoning about it from first principles, I ended going around in circles for a while.
The change look good including the notes regarding future fixes, which should further consolidate half baked analysis into a few purpose-built procedures.
/merge BTW, I made some very minor updates to the PR description for typos. |
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below:
|
Summary
Fix two issues with the method-call syntax in non-dirty templates:
binding (dot function call syntax inside a template can't lookup symbols of an imported module when the template is called from another module #1292)
Details
semTemplBody
restructuringRestructure
semTemplBody
so that theTemplCtx.noGenSym
fieldbecomes unnecessary:
nkExprColonExpr
/nkExprEqExpr
can only bind totemplate parameters, so
qualifiedLookUp
is used directlytemplBindSym
a.b
), handling symbol binding for theb
partis done manually via symbol lookup +
templBindSym
Missing early binding
b
ina.b
is a non-overloaded symbol, wrap it in a symbol-choice, to prevent
dotTransformation
from discarding it againcase, not a closed one (non-overloaded symbols are closed by
default)
knownIssue
test is added for the incorrect symbol binding in dotexpressions (
tclosed_symbol_with_method_call
)Bind
gensym
symbolsb
ina.b
resolves to a gensym, the symbols is now boundb
in
a.b
. This prevents, incorrect symbol binding in cases such asvar x = 1; a.x = 2
a.b
resolving to a field access even if a gensym wasbound to
b
(and later turned intob'gensym
identifier),builtInFieldAccess
andpropertyWriteAccess
strip the'gensym
suffix from the identifier before trying to use it
sem
as(DotExpr (...) (Ident 'suffix))
stays the same,renderer.isCustomLit
has to account for symbol-choices in thesecond slot
A bug with
symChoice
uncovered by the other changes is fixed: gensymswere added to the symbol choice, even if
isField
(now rename to themore accurately named
noGenSyms
) was true.Misc
are added