Skip to content

Commit

Permalink
sem: rework instantiation of object types (#876)
Browse files Browse the repository at this point in the history 
## Summary

Rework the instantiation logic for generic object types, and also
improve the early detection of illegal inheritance. The goal is
to make generic object types behave the same in all situations and
to have the implementation in a single place.

**Breaking changes:**
* the base type for an object may only use a single `ref`/`ptr`
  indirection; inheriting from a type like `ref ref Object` is now
  disallowed

**Fixes for parametric object types where none of the parameters
are used in the body:**
* type-bound operators for them now work the same as for other
  parametric object types, instead of all type instances using the same
  set of procedures 
* the static `of` operator doesn't treat all type instances thereof as
  the same type
* the dynamic `of` operator doesn't treat all type instance thereof as
  the same type, but only when using either the JS or VM backend; the
  problem still remains for the C backend
* `requiresInit` is now correctly inferred for them

**Other fixes:**
* `privateAccess Typ[int]` no longer allows access to the fields of all
  `Typ` type instances
* generic `object` types are now consistently checked for duplicate
  field names, regardless of how they're instantiated
* sharing field names with fields inside unused `when` branches now
  works consistently for generic `object` types
* the VM doesn't crash anymore when accessing the field of an
  indirectly instantiated `object` where the base type is not known
  early (e.g., because its a generic parameter)
* illegal inheritance (e.g., base is not an object) is now rejected in
  all cases, regardless of a generic `object` type is instantiated
* the compiler doesn't crash anymore when inheriting from an unresolved
  generic `object` type that has a type parameter as the base type
* "inheritance only works with non-final object type" errors are not
  reported when the specified base type is not an `object` type

**Improvements:**
* illegal inheritance for generic object types is now detected at
  definition-time in more complex cases

## Details

### Definition analysis

* add the `skipToObject` procedure to the `types` module. It replaces
  the previously used `skipGenericInvocation`, by default doesn't skip
  over more than one `ref|ptr` indirection, and also supports retrieving
  the object type in cases such as `Wrapper[T] = T; Wrapper[Obj[T]]`
* only add the fields of the base type(s) to the `check` set if the base
  is a concrete type (the comment stating that it already worked like
  this was incorrect)
* report an "is not a concrete type" error when the object type is not
  generic and the type specified as the base is a meta type

By using the `skipToObject` in `semObjectNode`, more cases of illegal
inheritance are detected on type definition and only a single `ptr|ref`
indirection is allowed, which enforces the updated inheritance rules.

Contrary to what the comment in `semObjectNode` stated, the fields of
the base type were added for checking (via `addInheritedFields`), even
when
the specified base type is generic, which resulted in both crashes (when
one of
the base type's base types was a `tyGenericParam`) and duplicate field
names that could be legal depending on the type arguments being rejected
too
eagerly.

### Instantiation rework

Many of the bugs regarding generic object types were caused by their
processing being scattered across multiple procedures (`semGeneric`,
`replaceTypeVarsTAux`, and `generateTypeInstance`), where not all
of them are called in all situations. For example, the most proper
handling (`semGeneric`) was only used when a generic is invoked
with concrete type arguments (e.g., `Generic[int]`).

To fix this the scattered logic is all moved into a single procedure
(`instantiate`), which replaces the `replaceTypeVarsT` call in
`handleGenericInvocation`. Since `handleGenericInvocation` is called
for all generic invocations, this makes sure that instantiation
always works the same. The new `instantiate` procedure also makes it
possible to, in the future, customize the behaviour for other types
created through invocations.

For `object` types, `instantiate` also verifies that the resolved base
type is valid. `replaceTypeVarsT` did not, meaning that object types
not instantiated
via `semGeneric` could previously use, after type parameter are
replaced,
any non-meta type as the base type without the compiler complaining
(although the compiler would have most likely crashed later on).

The `replaceObjBranches`, `recomputeFieldPositions`, and
`semObjectTypeForInheritedGenericInst` procedures together with the
`nkRecWhen` handling from `replaceTypeVarsN` are all merged together
into the new `instantiateRecord` procedure -- `addInheritedFields` and
`addInheritedFieldsAux` are simplified and moved into the `semtypinst`
module.

This means that `object` types are now always instantiated in the same
way, regardless of which path led up to the `handleGenericInvocation`
call.

There are two other important changes:
1. a new instance is always created, even if the `object` type doesn't
   reference any type parameters in its body
2. all instantiated `object` types get a unique `PSym` instead of re-
   using the generic type's one

The consequences are as follows. For the first point:
* `sameType` fully consider phantom object types now, which means that
  `inheritanceDiff` (used for computing the inheritance relationship
  between two objects outside of `sigmatch`) does too
* the `typeInst` field for all phantom object types where none of the
  parameters are used in the body points to the correct `tyGenericInst`,
  meaning that `hashType` (which always uses `typeInst`, if available)
  works correctly for all phantom object types
* the run-time object type identification for both VM and JS backend
  is based on the `PType`'s ID, meaning that the `of` operator for both
  backends now considers all phantom object types

For the second point:
* the `typ` field for a `PSym` of an instantiated `object` type now
  points back to the instantiated type instead of the generic type
* the `owner` for the fields of an instantiated `object` type points to
  instantiated type instead of the originating-from generic type

With `hashType` now always producing different hashes for different
phantom `object` types, each one gets its own set of type-bound
operators lifted. An optimization is possible (and implemented): since
each `tyObject` `PType` is guaranteed to be unique now, the
canonicalization through `hashType` is unnecessary and can be elided.

### Adjusted `privateAccess`

Since all instantiated object types now use a unique symbol instead of
the symbol of their originating-from `tyGenericBody`, `semPrivateAccess`
as currently implemented doesn't work anymore: the symbol of the generic
object (retrieved by `toObjectFromRefPtrGeneric`) is added to the "allow
access" list, but the `owner` of the instances' fields now points to the
instantiated type.

`toObjectFromRefPtrGeneric` is changed to retrieve the symbol of the
instance (if the provided type is a `tyGenericInst`), restoring the
basic functionality. For restoring support for "allow access to all
instances of generic type" , `fieldVisible`, for instantiated types
coming from generics, also checks if the symbol of the generic type is
in the "allow access" list.

### Changes to tests

* add tests for the fixed issues
* create a single test file for illegal inheritance detection
  (`tillegal_base_type.nim`) and merge the `tparameterizedparent0.nim`
  and `tparameterizedparent1.nim` tests into it

---------

Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
Co-authored-by: Clyybber <darkmine956@gmail.com>
  • Loading branch information
@zerbina @saem @Clyybber
3 people authored Sep 14, 2023
1 parent ce91535 commit c756da6
Show file tree
Hide file tree
Showing 18 changed files with 559 additions and 242 deletions.
7 changes: 4 additions & 3 deletions compiler/ast/ast.nim
View file
Original file line number Diff line number Diff line change
Expand Up @@ -607,9 +607,10 @@ proc toObjectFromRefPtrGeneric*(typ: PType): PType =
result = typ
while true:
case result.kind
of tyGenericBody: result = result.lastSon
of tyRef, tyPtr, tyGenericInst, tyGenericInvocation, tyAlias: result = result[0]
# automatic dereferencing is deep, refs #18298.
of tyGenericInvocation:
result = result[0]
of tyRef, tyPtr, tyGenericInst, tyAlias, tyGenericBody:
result = result[^1]
else: break
assert result.sym != nil

Expand Down
35 changes: 35 additions & 0 deletions compiler/ast/types.nim
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,41 @@ proc containsCompileTimeOnly*(t: PType): bool =
return true
return false

proc skipToObject*(t: PType; numIndirectsAllowed = 1): PType {.inline.} =
## Tries to skip to the underlying ``tyObject`` of a type that is used
## as an object's base type. Unresolved generic invocations are
## traversed in simple cases. Either the ``tyObject`` type or the first
## encountered type that couldn't be traversed is returned.
##
## `numIndirectsAllowed` specifies the number of ``ptr`` and ``ref``
## types to skip before bailing out.
case t.kind
of tyAlias, tyGenericInst:
skipToObject(t.lastSon, numIndirectsAllowed)
of tyRef, tyPtr:
# only skip ``ref``/``ptr`` indirections if allowed
if numIndirectsAllowed > 0:
skipToObject(t.lastSon, numIndirectsAllowed - 1)
else:
t
of tyGenericInvocation:
# also skip through invocations
let body = t.base.lastSon
if body.kind == tyObject or tfRefsAnonObj in body.flags:
skipToObject(body, numIndirectsAllowed)
elif body.kind in {tyGenericInvocation, tyGenericParam}:
var x = skipToObject(body, numIndirectsAllowed)
if x.kind == tyGenericParam:
# special case: for a ``type Typ[A] = A`` invoked through
# ``Typ[Other[T]]``, we traverse ``Other[T]`` instead of
# returning ``A``
x = skipToObject(t[1 + x.sym.position], numIndirectsAllowed)
x
else:
t # too complex; needs to be figured out by the caller
else:
t

proc safeSkipTypes*(t: PType, kinds: TTypeKinds): PType =
## same as 'skipTypes' but with a simple cycle detector.
result = t
Expand Down
15 changes: 11 additions & 4 deletions compiler/sem/liftdestructors.nim
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1026,11 +1026,18 @@ proc createTypeBoundOps(g: ModuleGraph; c: PContext; orig: PType; info: TLineInf
let skipped = orig.skipTypes({tyGenericInst, tyAlias, tySink})
if isEmptyContainer(skipped) or skipped.kind == tyStatic: return

let h = sighashes.hashType(skipped, {CoType, CoDistinct})
var canon = g.canonTypes.getOrDefault(h)
if canon == nil:
g.canonTypes[h] = skipped
var canon: PType
if skipped.kind == tyObject:
# for nominal types, the type itself is already the canonical one (each one
# is unique)
# XXX: ^^ at present, this is only true for object types. Phantom
# ``tyDistinct`` and ``tyEnum`` types still don't have unique
# instances
canon = skipped
else:
# structural types use canonicalization
canon = g.canonTypes.mgetOrPut(hashType(skipped, {CoType, CoDistinct}),
skipped)

# multiple cases are to distinguish here:
# 1. we don't know yet if 'typ' has a nontrival destructor.
Expand Down
13 changes: 12 additions & 1 deletion compiler/sem/lookups.nim
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1196,8 +1196,19 @@ proc fieldVisible*(c: PContext, f: PSym): bool {.inline.} =
if fmoduleId == module.id: return true
if f.kind == skField:
var symObj = f.owner
let genericOwner =
if sfFromGeneric in f.owner.flags:
f.owner.owner
else:
nil

if symObj.typ.kind in {tyRef, tyPtr}:
symObj = symObj.typ[0].sym
# go through all scopes and check whether access to the object is
# allowed. The generic owner test is needed for the "access to all
# instances of generic object type allowed" feature to work
for scope in allScopes(c.currentScope):
for sym in scope.allowPrivateAccess:
if symObj.id == sym.id: return true
if symObj.id == sym.id or
(genericOwner != nil and genericOwner.id == sym.id):
return true
5 changes: 0 additions & 5 deletions compiler/sem/semobjconstr.nim
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,11 +339,6 @@ proc checkConstructTypeAux(c: PContext,
let base = t[0]
if base == nil: break
t = skipTypes(base, skipPtrs)
if t.kind != tyObject:
# XXX: This is not supposed to happen, but apparently
# there are some issues in semtypinst. Luckily, it
# seems to affect only `computeRequiresInit`.
return
constrCtx.needsFullInit = constrCtx.needsFullInit or
tfNeedsFullInit in t.flags

Expand Down
132 changes: 38 additions & 94 deletions compiler/sem/semtypes.nim
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1048,89 +1048,60 @@ proc semRecordNodeAux(c: PContext, n: PNode, check: var IntSet, pos: var int,

styleCheckDef(c.config, f)
if a.kind != nkEmpty: father.add a
of nkSym:
# This branch only valid during generic object
# inherited from generic/partial specialized parent second check.
# There is no branch validity check here
if containsOrIncl(check, n.sym.name.id):
localReport(c.config, n.info, reportSym(rsemRedefinitionOf, n.sym))

father.add n
of nkEmpty:
if father.kind in {nkElse, nkOfBranch}:
father.add n
else:
semReportIllformedAst(c.config, n, "?")

proc addInheritedFieldsAux(c: PContext, check: var IntSet, pos: var int,
n: PNode) =
case n.kind
of nkRecCase:
c.config.internalAssert(n[0].kind == nkSym, n.info, "addInheritedFieldsAux")
addInheritedFieldsAux(c, check, pos, n[0])
for i in 1..<n.len:
case n[i].kind
of nkOfBranch, nkElse:
addInheritedFieldsAux(c, check, pos, lastSon(n[i]))
else:
internalError(c.config, n.info, "addInheritedFieldsAux(record case branch)")
of nkRecList, nkRecWhen, nkElifBranch, nkElse:
for i in int(n.kind == nkElifBranch)..<n.len:
addInheritedFieldsAux(c, check, pos, n[i])
of nkSym:
incl(check, n.sym.name.id)
inc(pos)
else:
internalError(c.config, n.info, "addInheritedFieldsAux()")

proc skipGenericInvocation(t: PType): PType {.inline.} =
result = t
if result.kind == tyGenericInvocation:
result = result[0]
while result.kind in {tyGenericInst, tyGenericBody, tyRef, tyPtr, tyAlias, tySink}:
result = lastSon(result)

proc addInheritedFields(c: PContext, check: var IntSet, pos: var int,
obj: PType) =
assert obj.kind == tyObject, $obj.kind
if (obj.len > 0) and (obj[0] != nil):
addInheritedFields(c, check, pos, obj[0].skipGenericInvocation)
addInheritedFieldsAux(c, check, pos, obj.n)

proc semObjectNode(c: PContext, n: PNode, prev: PType; flags: TTypeFlags): PType =
if n.len == 0:
return newConstraint(c, tyObject)
var check = initIntSet()
var pos = 0
var base, realBase: PType = nil
var realBase: PType = nil
# n[0] contains the pragmas (if any). We process these later...
checkSonsLen(n, 3, c.config)
if n[1].kind != nkEmpty:
realBase = semTypeNode(c, n[1][0], nil)
base = skipTypesOrNil(realBase, skipPtrs)
if base.isNil:
localReport(c.config, n, reportSem rsemExpectObjectForBase)
else:
var concreteBase = skipGenericInvocation(base)
if concreteBase.kind in {tyObject, tyGenericParam,
tyGenericInvocation} and tfFinal notin concreteBase.flags:
# we only check fields duplication of object inherited from
# concrete object. If inheriting from generic object or partial
# specialized object, there will be second check after instantiation
# located in semGeneric.
if concreteBase.kind == tyObject:
if concreteBase.sym != nil and concreteBase.sym.magic == mException and
sfSystemModule notin c.module.flags:
localReport(c.config, n.info, reportSem(rsemInheritFromException))

addInheritedFields(c, check, pos, concreteBase)
let base = skipToObject(realBase)
case base.kind
of tyObject:
if tfFinal notin base.flags:
# we only check field duplication for objects inheriting from
# concrete objects. If inheriting from a generic object or partially
# specialized object, the duplication check is performed during
# instantiation
if realBase.skipTypes(skipPtrs).kind == tyObject:
pos += addInheritedFields(check, base)

if base.sym != nil and base.sym.magic == mException and
sfSystemModule notin c.module.flags:
localReport(c.config, n.info, reportSem(rsemInheritFromException))
else:
if concreteBase.kind != tyError:
localReport(c.config, n[1].info, reportTyp(
rsemExpectNonFinalForBase, realBase))

base = nil
localReport(c.config, n[1].info):
reportTyp(rsemExpectNonFinalForBase, realBase)
realBase = nil
of tyGenericParam:
# the base is a generic parameter (e.g., ``object of T``)
discard "figured out later"
of tyError:
# don't cascade errors
realBase = nil
elif base.isMetaType:
if c.inGenericContext > 0:
discard "okay, handled during instantiation"
else:
localReport(c.config, n[1].info):
reportTyp(rsemTIsNotAConcreteType, realBase)
realBase = nil
else:
# not something that can be inherited from
localReport(c.config, n[1].info):
reportTyp(rsemExpectObjectForBase, realBase)
realBase = nil

c.config.internalAssert(n.kind == nkObjectTy, n.info, "semObjectNode")
result = newOrPrevType(tyObject, prev, c)
rawAddSon(result, realBase)
Expand All @@ -1141,7 +1112,7 @@ proc semObjectNode(c: PContext, n: PNode, prev: PType; flags: TTypeFlags): PType
result.n = newNodeI(nkRecList, n.info)
else:
# partial object so add things to the check
addInheritedFields(c, check, pos, result)
pos += addInheritedFields(check, result)
if not incompleteType(result):
# this record AST represents the final addition to the object; the
# size can be computed now
Expand All @@ -1158,7 +1129,7 @@ proc semObjectNode(c: PContext, n: PNode, prev: PType; flags: TTypeFlags): PType
for e in ifErrorWalkErrors(c.config, n[0]):
localReport(c.config, e)

if base == nil and tfInheritable notin result.flags:
if realBase == nil and tfInheritable notin result.flags:
incl(result.flags, tfFinal)

if c.inGenericContext == 0 and computeRequiresInit(c, result):
Expand Down Expand Up @@ -1692,28 +1663,6 @@ proc semGenericParamInInvocation(c: PContext, n: PNode): PType =
result = semTypeNode(c, n, nil)
n.typ = makeTypeDesc(c, result)

proc semObjectTypeForInheritedGenericInst(c: PContext, n: PNode, t: PType) =
var
check = initIntSet()
pos = 0
let
realBase = t[0]
base = skipTypesOrNil(realBase, skipPtrs)
if base.isNil:
localReport(c.config, n.info, reportTyp(rsemIllegalRecursion, t))

else:
let concreteBase = skipGenericInvocation(base)
if concreteBase.kind == tyObject and tfFinal notin concreteBase.flags:
addInheritedFields(c, check, pos, concreteBase)
else:
if concreteBase.kind != tyError:
localReport(c.config, n.info, reportTyp(
rsemExpectNonFinalForBase, concreteBase))

var newf = newNodeI(nkRecList, n.info)
semRecordNodeAux(c, t.n, check, pos, newf, t)

proc semGeneric(c: PContext, n: PNode, s: PSym, prev: PType): PType =
if s.typ == nil:
localReport(c.config, n.info, reportSym(
Expand Down Expand Up @@ -1792,11 +1741,6 @@ proc semGeneric(c: PContext, n: PNode, s: PSym, prev: PType): PType =
rsemIllegalRecursion, result[0]))

return errorType(c)
if tx != result and tx.kind == tyObject:
if tx[0] != nil:
semObjectTypeForInheritedGenericInst(c, n, tx)
var position = 0
recomputeFieldPositions(tx, tx.n, position)

proc maybeAliasType(c: PContext; typeExpr, prev: PType): PType =
if typeExpr.kind in {tyObject, tyEnum, tyDistinct, tyForward, tyGenericBody} and prev != nil:
Expand Down
Loading

0 comments on commit c756da6

Please sign in to comment.