Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 140 out of 142 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
in sym collector
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 140 out of 142 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
drift-core/src/main/kotlin/drift/runtime/ParserType.kt:246
ArrayType.asString()currently returns"($type[])", which usestype.toString()(e.g.,ObjectType(className=Int, ...)) and adds parentheses, producing unstable/non-language type strings. Usetype.asString()and format as${type.asString()}[](or whatever the Drift surface syntax is) for consistent user-facing type rendering.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 140 out of 142 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
drift-core/src/main/kotlin/drift/runtime/ParserType.kt:247
ArrayType.asString()uses string interpolation ontypedirectly ("($type[])"). This will calltype.toString()(e.g.,ObjectType(className=...)) rather thantype.asString(), producing incorrect/unstable type names. Build the string fromtype.asString()instead (and consider dropping the surrounding parentheses unless they’re required by the language grammar).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| fun hasClass(name: String) : Boolean = | ||
| scopes.last().bindings.containsKey(name) |
There was a problem hiding this comment.
SymbolTable.hasClass only checks the current (last) scope. Because SymbolCollector pushes/pops scopes for blocks/functions, this will incorrectly report global classes as missing when type-checking inside nested scopes. Make hasClass search all scopes (similar to lookupNodeId) or delegate to lookupNodeId(name) != null and ensure it only returns true for class symbols.
| scopes.last().bindings.containsKey(name) | |
| lookupNodeId(name)?.let { allSymbols[it] is ClassSymbol } ?: false |
| is Token.Annotation -> { | ||
| val annotation = parseAnnotation() | ||
| storedAnnotations.add(annotation) | ||
|
|
||
| parseClassStatement() | ||
| } |
There was a problem hiding this comment.
In parseClassStatement, the Token.Annotation branch recurses immediately after parseAnnotation() without skipping a newline. If the annotation is on its own line (common style), current() will be Token.NewLine and the recursive call will throw DPUnexpectedStatementInClassBodyException. Add skip(Token.NewLine) (or handle Token.NewLine in the dispatcher) before recursing so both @Anno member and @Anno\nmember work.
| if (matchSymbol("{")) { | ||
| while (!matchSymbol("}")) { | ||
| skip(Token.NewLine) | ||
|
|
||
| val c = current() | ||
|
|
||
| if (c is Token.Identifier) { | ||
| when { | ||
| c.isKeyword(Token.Keyword.INIT) -> { | ||
| if (hasPrimaryConstructor) { | ||
| throw DPOnlyOneConstructorPerClassException() | ||
| } | ||
|
|
||
| methods.add(parseHook( | ||
| forceParameters = true, | ||
| disableReturnStatement = true)) | ||
| } | ||
| c.isKeyword(Token.Keyword.IMMUTLET) || | ||
| c.isKeyword(Token.Keyword.MUTLET) -> { | ||
|
|
||
| advance(false) | ||
|
|
||
| fields += parseLet( | ||
| isMutable = c.isKeyword(Token.Keyword.MUTLET), | ||
| acceptUnassigned = true) | ||
| } | ||
| c.isKeyword(Token.Keyword.FUNCTION) -> { | ||
| advance() | ||
|
|
||
| methods.add(parseFunction()) | ||
| } | ||
| c.isKeyword(Token.Keyword.STATIC) -> { | ||
| if (isStaticBlockAlreadyDefined) | ||
| throw DPOnlyOneStaticBlockPerClassException() | ||
|
|
||
| isStaticBlockAlreadyDefined = true | ||
|
|
||
| advance() | ||
|
|
||
| expectSymbol("{", advanceOnSuccess = true) | ||
|
|
||
| while (!matchSymbol("}")) { | ||
| skip(Token.NewLine) | ||
|
|
||
| parseClassStaticBlock(staticFields, staticMethods) | ||
| } | ||
| } | ||
| else -> throw DPUnexpectedStatementInClassBodyException() | ||
| } | ||
| } else { | ||
| throw DPUnexpectedStatementInClassBodyException() | ||
| } | ||
| parseClassStatement() | ||
|
|
||
| if (current() is Token.NewLine) advance() | ||
| advance() | ||
| } |
There was a problem hiding this comment.
The class-body loop calls advance() unconditionally after parseClassStatement(). Since the parsing routines already advance as they consume tokens, this extra advance can skip over the closing } or the first token of the next member, desynchronizing parsing. Remove this advance() and rely on skip(Token.NewLine) / matchSymbol("}") to drive the loop (similar to parseBlock()).
Changelogs
drift-bootstrapmoduledrift-irfor potential future QBE backend, in stand by