Skip to content
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

Symbol.for('bar').toString() missing 'bar' (missing [[Description]] value) #621

Closed
dckc opened this issue Apr 2, 2021 · 4 comments
Closed
Labels
confirmed issue reported has been reproduced fixed - please verify Issue has been fixed. Please verify and close.

Comments

@dckc
Copy link
Contributor

dckc commented Apr 2, 2021

Steps to Reproduce

xst -e "print(Symbol.for('bar').toString())"
Symbol()

Expected behavior

Symbol(bar)

Other information

ref https://tc39.es/ecma262/#sec-symbol.prototype.tostring

context: q as best efforts stringify test
https://github.com/endojs/endo/blob/master/packages/ses/test/error/test-assert-log.js#L421
endojs/endo#508

cc @erights @kriskowal

@phoddie phoddie added the confirmed issue reported has been reproduced label Apr 2, 2021
@phoddie
Copy link
Collaborator

phoddie commented Apr 2, 2021

Interesting find. A quick fix is to modify fxSymbolToString:

void fxSymbolToString(txMachine* the, txSlot* slot)
{
	txSlot* key = fxGetKey(the, slot->value.symbol);
	fxStringX(the, slot, "Symbol(");
	if (key) {
		if ((key->kind == XS_STRING_KIND) || (key->kind == XS_STRING_X_KIND))
			fxConcatStringC(the, slot, key->value.string);
		else if ((key->kind == XS_KEY_KIND) || (key->kind == XS_KEY_X_KIND))
			fxConcatStringC(the, slot, key->value.key.string);
	}
	fxConcatStringC(the, slot, ")");
}

A more careful review and a run through test262 will be needed for a real fix.

@phoddie
Copy link
Collaborator

phoddie commented Apr 2, 2021

(Apparently test262 didn't report this? If that's true, we should let the maintainers of teset262 know.)

@erights
Copy link

erights commented Apr 2, 2021 via email

@dckc
Copy link
Contributor Author

dckc commented Apr 14, 2021

fix confirmed.

connolly@jambox:~/projects/agoric/endo-mod-up/packages/ses$ yarn test:xs test/error/test-assert-log.js 
yarn run v1.22.0
$ ava-xs test/error/test-assert-log.js
12 tests passed
Done in 1.40s.

@phoddie phoddie added the fixed - please verify Issue has been fixed. Please verify and close. label Apr 14, 2021
@phoddie phoddie closed this as completed Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed issue reported has been reproduced fixed - please verify Issue has been fixed. Please verify and close.
Projects
None yet
Development

No branches or pull requests

3 participants