-
-
Notifications
You must be signed in to change notification settings - Fork 670
Change assert to allow blank strings #1177
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.
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
Conversation
a067028
to
7d65a8d
Compare
src/program.ts
Outdated
@@ -1044,7 +1044,9 @@ export class Program extends DiagnosticEmitter { | |||
// TODO: for (let [alias, name] of globalAliases) { | |||
for (let _keys = Map_keys(globalAliases), i = 0, k = _keys.length; i < k; ++i) { | |||
let alias = unchecked(_keys[i]); | |||
let name = assert(globalAliases.get(alias)); | |||
let name = globalAliases.get(alias); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just use changetype<string>(globalAliases.get(alias))
since the value can't be null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it can be undefined right?
alias = "bad key"
let name = changetype<string>(globalAliases.get(alias))
if (!name.length) continue; // explicitly disabled
throws
ERROR: TypeError: Cannot read property 'length' of undefined
I suppose that's not really much different than the assertion though and shouldn't really happen anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah, that's strange. Might be cli/asc.js
wrongly inserting an undefined
into the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, alias = "bad key"
doesn't happen since we _keys = Map_keys(globalAliases)
so we know the key is in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay- well in that case maybe we should just use !
and ignore the undefined case
Not sure I understand why CI is failing here- doesn't appear to be related to the PR |
Seems to me that there's something wrong with GitHub's git, hmm. Will try to rerun the CI job at a later time. |
Is this blocked on CI failures? |
e5c80e8
to
daf9524
Compare
daf9524
to
6cf7b7d
Compare
CI is passing now, should be good to merge? |
Issue: #1176
When using the documented
--use abort=
abort would be set to an empty string, which resulted in the call to global aliases returning ''. Then since blank strings are falsey, this assertion would fail.I've changed the assertion to assert that the name is a string, which catches the case that the alias is missing but does not fail in the case of an empty string
examples:
If the name isn't found in the aliases table:
We get:
ERROR: AssertionError: assertion failed
as expected.In the case where you specify an abort that does not match any export:
ERROR: no such global element: main/nope
as beforeIn the case where you use a blank abort, compilation is successful.