Skip to content

Commit

Permalink
Add the ability for errors messages to link to error explanations (#1789
Browse files Browse the repository at this point in the history
)

* compiler errors have linkable codes

* don'e use regex to detect error tags

* fix broken error message

* change error to not look like tag

* fix test to match error
  • Loading branch information
mtoy-googly-moogly authored Jul 22, 2024
1 parent 43bfaa1 commit 6227872
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 7 deletions.
4 changes: 2 additions & 2 deletions packages/malloy/src/lang/ast/expressions/expr-func.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,9 +350,9 @@ export class ExprFunc extends ExpressionDef {
? `'.' paths are not yet supported in sql interpolations, found ${unsupportedInterpolations.at(
0
)}`
: `'.' paths are not yet supported in sql interpolations, found [${unsupportedInterpolations.join(
: `'.' paths are not yet supported in sql interpolations, found (${unsupportedInterpolations.join(
', '
)}]`;
)})`;
this.log(unsupportedInterpolationMsg);

return errorFor(
Expand Down
14 changes: 10 additions & 4 deletions packages/malloy/src/lang/ast/expressions/pick-when.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@ export class Pick extends ExpressionDef {
);
if (returnType && !FT.typeEq(returnType, thenExpr, true)) {
const whenType = FT.inspect(thenExpr);
this.log(`pick type '${whenType}', expected '${returnType.dataType}'`);
this.log(
`pick type '${whenType}', expected '${returnType.dataType}'[pick-values-must-match]`
);
return errorFor('pick when type');
}
returnType = typeCoalesce(returnType, thenExpr);
Expand All @@ -114,7 +116,7 @@ export class Pick extends ExpressionDef {
this.log(
`${errSrc} type '${FT.inspect(elseVal)}', expected '${
returnType.dataType
}'`
}'[pick-values-must-match]`
);
return errorFor('pick else type');
}
Expand Down Expand Up @@ -164,7 +166,9 @@ export class Pick extends ExpressionDef {
}
if (returnType && !FT.typeEq(returnType, aChoice.pick, true)) {
const whenType = FT.inspect(aChoice.pick);
this.log(`pick type '${whenType}', expected '${returnType.dataType}'`);
this.log(
`pick type '${whenType}', expected '${returnType.dataType}'[pick-values-must-match]`
);
return errorFor('pick value type');
}
returnType = typeCoalesce(returnType, aChoice.pick);
Expand Down Expand Up @@ -196,7 +200,9 @@ export class Pick extends ExpressionDef {
returnType = typeCoalesce(returnType, defVal);
if (!FT.typeEq(returnType, defVal, true)) {
this.elsePick.log(
`else type '${FT.inspect(defVal)}', expected '${returnType.dataType}'`
`else type '${FT.inspect(defVal)}', expected '${
returnType.dataType
}'[pick-values-must-match]`
);
return errorFor('pick value type mismatch');
}
Expand Down
17 changes: 17 additions & 0 deletions packages/malloy/src/lang/parse-log.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export interface LogMessage {
message: string;
at?: DocumentLocation;
severity: LogSeverity;
errorTag?: string;
}

export interface MessageLogger {
Expand All @@ -50,7 +51,23 @@ export class MessageLog implements MessageLogger {
return this.rawLog;
}

/**
* Add a message to the log.
*
* If the messsage ends with '[tag]', the tag is removed and stored in the `errorTag` field.
* @param logMsg Message possibly containing an error tag
*/
log(logMsg: LogMessage): void {
const msg = logMsg.message;
// github security is worried about msg.match(/^(.+)\[(.+)\]$/ because if someone
// could craft code with a long varibale name which would blow up that regular expression
if (msg.endsWith(']')) {
const tagStart = msg.lastIndexOf('[');
if (tagStart > 0) {
logMsg.message = msg.slice(0, tagStart);
logMsg.errorTag = msg.slice(tagStart + 1, -1);
}
}
this.rawLog.push(logMsg);
}

Expand Down
7 changes: 7 additions & 0 deletions packages/malloy/src/lang/test/parse.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1026,3 +1026,10 @@ describe('sql_functions', () => {

test('non breaking space in source', () =>
expect('source:\u00a0z\u00a0is\u00a0a').toParse());

test('error tagging', () => {
const m = model`run: a -> { select: err is pick 1 when true else 'one' }`;
expect(m).translationToFailWith("else type 'string', expected 'number'");
const firstErr = m.translator.root.logger.getLog()[0];
expect(firstErr?.errorTag).toBe('pick-values-must-match');
});
2 changes: 1 addition & 1 deletion test/src/databases/all/expr.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ describe.each(runtimes.runtimeList)('%s', (databaseName, runtime) => {
`
);
await expect(query.run()).rejects.toThrow(
"'.' paths are not yet supported in sql interpolations, found [${a.seats}, ${a.seats}, ${a.total_seats}]"
"'.' paths are not yet supported in sql interpolations, found (${a.seats}, ${a.seats}, ${a.total_seats})"
);
});

Expand Down

0 comments on commit 6227872

Please sign in to comment.