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

Fix incorrect function.name for aliased imports #1484

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

RichDom2185
Copy link
Member

Uses Object.defineProperty to set the correct function.name value for aliased imports in:

  • ec-evaluator
  • transpiler

Did not do so for interpreter/interpreter.ts due to the deprecation note in #1476.

How to test

Consider the following program:

import { play as asdf } from 'sound';

asdf("test");

Previously, this will result in the following error:

Error: play is expecting sound, but encountered test

Now, it will result in:

Error: asdf is expecting sound, but encountered test

Tested with:

  • Source §1/2/3/4
  • Source §1/2/3/4 Native
  • Source §1/2/3/4 Typed
  • Source §1/2 Lazy
  • Source §4 Explicit-Control
  • Full JS

@RichDom2185 RichDom2185 self-assigned this Sep 10, 2023
@RichDom2185 RichDom2185 enabled auto-merge (squash) September 10, 2023 09:41
@RichDom2185 RichDom2185 enabled auto-merge (squash) September 10, 2023 09:44
* Replace typecase with `satisfies` operator
* Update return type
Copy link
Member Author

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

I just have some doubts on the code quality as follows:

@@ -268,6 +268,7 @@ function prepareBuiltins(oldBuiltins: Map<string, any>) {
}
}
newBuiltins.set('undefined', undefined)
newBuiltins.set('Object', Object)
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is the right move; what are the implications of this? Perhaps someone could verify/advise.

Copy link
Member

Choose a reason for hiding this comment

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

I see that you are updating the name property in the function object. But here, are you trying to add the name "Object" to the names of builtins that are visible from Source programs?

Comment on lines +590 to +592
const importedNames = (
importsToAdd.filter(node => node.type === 'VariableDeclaration') as es.VariableDeclaration[]
).flatMap(node =>
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if there's a less hacky way to handle this...

`satisfies` is unfortunately only introduced in TS 4.9.
@coveralls
Copy link

coveralls commented Sep 10, 2023

Pull Request Test Coverage Report for Build 7593472306

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 82.936%

Totals Coverage Status
Change from base Build 7593229382: -0.001%
Covered Lines: 10896
Relevant Lines: 12709

💛 - Coveralls

@leeyi45
Copy link
Contributor

leeyi45 commented Jan 22, 2024

If multiple files alias the same import the name information is lost

// a.js
import { show as s } from 'rune';
export function a(x) {
  return s(x);
}

// b.js
import { show as b } from 'rune';
import { a } from './a.js';

b(0);
a(0);

would theoretically cause the second call to a to display the function's name as 'b' (or vice versa). However, right now we only use the Function.name syntax when throwing errors, so this isn't an issue at the present moment

@RichDom2185 RichDom2185 marked this pull request as draft March 19, 2024 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants