Skip to content

Catch duplicate instance and static members #797

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Catch duplicate instance and static members #797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69418c6
Catch duplicate instance and static members
krznbtni Aug 28, 2019
6fd84e4
Fixed contrib
krznbtni Aug 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NOTICE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ under the licensing terms detailed in LICENSE:
* Aaron Turner <aaron@aaronthedev.com>
* Willem Wyndham <willem@cs.umd.edu>
* Bowen Wang <bowen@nearprotocol.com>
* Daniel Reed <daniel@reedcode.co.uk>
* Karzan Botani <karzan@botani.nu>

Portions of this software are derived from third-party works licensed under
the following terms:
Expand Down
1 change: 1 addition & 0 deletions src/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1651,6 +1651,7 @@ export class Compiler extends DiagnosticEmitter {
break;
}
case NodeKind.FIELDDECLARATION: {
if (!this.program.elementsByDeclaration.has(<FieldDeclaration>statement)) { break; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if expression and it's body contain in same line braces is unnecessary

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed since it didn't fail a lint that it wasn't enforced either way, if there is a preference would it be reasonable to add the check into a lint rule on a new PR?

let element = this.program.getElementByDeclaration(<FieldDeclaration>statement);
if (element.kind == ElementKind.GLOBAL) { // static
if (!element.hasDecorator(DecoratorFlags.LAZY)) this.compileGlobal(<Global>element);
Expand Down
4 changes: 4 additions & 0 deletions src/diagnosticMessages.generated.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ export enum DiagnosticCode {
_0_must_be_a_power_of_two = 223,
Expression_is_unsafe = 224,
Expression_is_never_null = 225,
Duplicate_instance_member_0 = 231,
Duplicate_static_member_0 = 232,
Unterminated_string_literal = 1002,
Identifier_expected = 1003,
_0_expected = 1005,
Expand Down Expand Up @@ -176,6 +178,8 @@ export function diagnosticCodeToString(code: DiagnosticCode): string {
case 223: return "'{0}' must be a power of two.";
case 224: return "Expression is unsafe.";
case 225: return "Expression is never 'null'.";
case 231: return "Duplicate instance member '{0}'.";
case 232: return "Duplicate static member '{0}'.";
case 1002: return "Unterminated string literal.";
case 1003: return "Identifier expected.";
case 1005: return "'{0}' expected.";
Expand Down
2 changes: 2 additions & 0 deletions src/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
"'{0}' must be a power of two.": 223,
"Expression is unsafe.": 224,
"Expression is never 'null'.": 225,
"Duplicate instance member '{0}'.": 231,
"Duplicate static member '{0}'.": 232,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have so far preferred to use TS errors if these are available, and only resort to use custom errors where there's no alternative. In this case, I think that "Duplicate identifier", which TS already has, transports the necessary information already - or doesn't it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaxGraey was the one who mentioned AS2301 on the issue, so we went with that.

If we use duplicate identifier, it doesn't tell us why it's a duplicate, surely?

If you'd rather use duplicate identifier, I think we would need to adjust the PR since we had to silence/replace some duplicate identifier errors due to what was said in the issue @dcodeIO .

Copy link
Member

@dcodeIO dcodeIO Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the "why" I'd say that a better solution for this particular concern would be to implement error chains eventually, indicating the conflicting declaration in addition to the actual error. Like, emitting a "duplicate identifier" here isn't so different from emitting it elsewhere, for example if there are multiple functions of the same name. That also doesn't give us detailed information, so using the error here as well is consistent.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So would you rather we revert this PR (and just fix the bug in the duplicate error check) or extend this PR to cover all duplicate identifier errors and convert them to more logical ones?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just suggesting to revert the custom AS errors, and use the existing duplicate identifier error instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is fixing the bug that is listed in the connected issue (#793)
that is fixed by https://github.com/AssemblyScript/assemblyscript/pull/797/files#diff-d5edbe17e79bd7d0417b0bac7e92be06R1654

The change from duplicate identifier was down to #793 (comment)

So is there an agreement between you and @MaxGraey as to what you want the error to be?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically only that line (and the test cases possibly) are needed to fix the assertion error, but we added the other messages on advice from @MaxGraey

Copy link
Member

@MaxGraey MaxGraey Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcodeIO TS inherit JS behaviour which call everything as identifier (class/instance member, fields in object and simple identifiers of variables and constants). I guess will be great have more precise diagnostic for such entities. Probably we could do soming like duplicate identifier (instance member), duplicate identifier (static member) and duplicate identifier for rest cases. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to align with TS where possible to provide a unified developer experience. Might also have implications on tooling that expect those exact TS error numbers for some reason. Ideally, we'd use TS errors like "Duplicate Identifier" but also implement error chains to highlight the conflicting identifier. Afaik TS has something like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, error chains could concrete errors. Nice


"Unterminated string literal.": 1002,
"Identifier expected.": 1003,
Expand Down
17 changes: 12 additions & 5 deletions src/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2038,10 +2038,17 @@ export abstract class Element {
if (merged) {
element = merged; // use merged element
} else {
this.program.error(
DiagnosticCode.Duplicate_identifier_0,
element.identifierNode.range, element.identifierNode.text
);
if (element.is(CommonFlags.STATIC)) {
this.program.error(
DiagnosticCode.Duplicate_static_member_0,
element.identifierNode.range, element.identifierNode.text
);
} else {
this.program.error(
DiagnosticCode.Duplicate_identifier_0,
element.identifierNode.range, element.identifierNode.text
);
}
return false;
}
}
Expand Down Expand Up @@ -3040,7 +3047,7 @@ export class ClassPrototype extends DeclaredElement {
let merged = tryMerge(instanceMembers.get(name)!, element);
if (!merged) {
this.program.error(
DiagnosticCode.Duplicate_identifier_0,
DiagnosticCode.Duplicate_instance_member_0,
element.identifierNode.range, element.identifierNode.text
);
return false;
Expand Down
10 changes: 10 additions & 0 deletions tests/compiler/class-duplicate.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"asc_flags": [
"--runtime none"
],
"stderr": [
"AS231: Duplicate instance member 'field'.",
"AS232: Duplicate static member 'static_field'.",
"EOF"
]
}
13 changes: 13 additions & 0 deletions tests/compiler/class-duplicate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// ERROR("SOF");

class FailureTest {
field: i64;
field: u32;

static static_field: string;
static static_field: i32;
}

const fail = new FailureTest();

ERROR("EOF");