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.

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

Jump to bottom

BREAKING CHANGE: Avoid collisions between static class/namespace members #2804

Closed
wants to merge 1 commit into from
Closed

BREAKING CHANGE: Avoid collisions between static class/namespace members #2804

wants to merge 1 commit into from

Conversation

CountBleck
Copy link
Member

Fixes #2793.

Changes proposed in this pull request:
⯈ Change the delimiter between namespaces and their members from . to ::

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@CountBleck
BREAKING CHANGE: Avoid collisions between static class/namespace members
11cb13b 
This commit changes the delimiter between namespaces and their members
from "." to "::" in internal names. The new delimiter certainly feels
very un-JavaScript-y, but the meaning is still recognizable, and these
names aren't exposed to users in most cases anyway. Still, this is a
breaking change, as it affects the import names for `declare namespace`
members (as seen in tests/compiler/declare.js), and transform authors
will also need to update their code accordingly.

Fixes AssemblyScript#2793.
@CountBleck CountBleck requested review from HerrCai0907 and MaxGraey and removed request for HerrCai0907 November 20, 2023 05:09
HerrCai0907
HerrCai0907 reviewed Nov 20, 2023
View reviewed changes
Copy link
Member

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

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

should we do diagnose first before merge this PR?

class NS {
  static v: i32 = 2;
}

namespace NS {
  export let v: i32 = 1;
}

TS playground

@CountBleck
Copy link
Member Author

should we do diagnose first before merge this PR?

Actually, this makes me realize how redundant this PR is, since if that diagnostic is emitted, then there wouldn't be a collision.

@CountBleck CountBleck closed this Nov 20, 2023
@CountBleck
Copy link
Member Author

CountBleck commented Nov 20, 2023 •
edited
Loading

Never mind, the diagnostic is only needed for exported namespace members.

@CountBleck CountBleck reopened this Nov 20, 2023
@CountBleck
Copy link
Member Author

@MaxGraey If you come up with a better delimiter, let me know. : wouldn't work since it could collide with getters and setters. I think all the good options have been taken.

Separately, it looks like TS supports namespaces being redeclared with non-exported members (of different types too), and it's probably easier if AS doesn't support that for now (just from looking at how these declarations are merged).

@CountBleck
Copy link
Member Author

It looks like supporting this properly is a pain, since modifying @HerrCai0907's example to remove the export still wouldn't work with this PR because of how declarations are merged. Accesses to NS.v end up targeting the namespace's v member, whether or not that member is exported.

It's probably best to diagnose this in every case.

@CountBleck CountBleck closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HerrCai0907 HerrCai0907 HerrCai0907 left review comments

@MaxGraey MaxGraey Awaiting requested review from MaxGraey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

asc crash when use same name in class and namespace
2 participants
@CountBleck @HerrCai0907