Catch duplicate instance and static members #797
Conversation
|
You should also discard dist folder from PR. |
|
Just done it, waiting on CI now |
| @@ -1651,6 +1651,7 @@ export class Compiler extends DiagnosticEmitter { | |||
| break; | |||
| } | |||
| case NodeKind.FIELDDECLARATION: { | |||
| if (!this.program.elementsByDeclaration.has(<FieldDeclaration>statement)) { break; } | |||
There was a problem hiding this comment.
nit: if expression and it's body contain in same line braces is unnecessary
There was a problem hiding this comment.
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?
| @@ -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, | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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 .
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Just suggesting to revert the custom AS errors, and use the existing duplicate identifier error instead.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, error chains could concrete errors. Nice
|
Yes, this should have been implemented meanwhile along related ranges. Closing this PR as part of 2020 vacuum. |
Added checks to report the duplicate fields in the locations they are already caught
Seems all the tests passed find when we did it, we added a test case for it too
Closes: #793
Fixing the contrib file and dist/* folder now, missed those in the push.