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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Set imported binding as an alias of target symbol #1193

Merged
merged 10 commits into from
May 26, 2024

Conversation

SPGoding
Copy link
Member

Fixes #1154.

mcdoc-binding.mp4

The file-level binding introduced by use is now treated as an alias of the global symbol.

The file-level binding is now treated as an alias of the global symbol.
packages/core/src/symbol/SymbolUtil.ts Show resolved Hide resolved
packages/mcdoc/src/binder/index.ts Outdated Show resolved Hide resolved
packages/mcdoc/src/binder/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@NeunEinser NeunEinser left a comment

Choose a reason for hiding this comment

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

This does not fully fix #1154.

The symbol deployed to mcdoc still contains the non-aliased reference.

If I delete the visibility: SymbolVisibility.File, in line 271, and edit my code to also do symbol.resolveAlias() when performing the lookup, I do get the correct node with the correct typedef.

I am not sure it is desirable to declare a public alias here though, I think I would prefer it if the reference type used the resolved path, and the alias wasn't public.

@SPGoding
Copy link
Member Author

SPGoding commented May 24, 2024

The symbol deployed to mcdoc still contains the non-aliased reference.

I think that's solved now. I also added a test that shows the global symbol instead of the file-level alias symbol is stored in the symbol table.

Copy link
Contributor

@NeunEinser NeunEinser left a comment

Choose a reason for hiding this comment

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

Nice, this works like a charm and no cluttering the global symbol table with import aliases =)

@NeunEinser
Copy link
Contributor

Btw, you can use my NeunEinser:neun branch which is a combination of SPG's branch and my mcdoc branch for testing and compare errors with just the NeunEinser:mcdoc-validator branch on its own to see it working.

Alias requires the target symbol to exist and can
dereference multi-layer aliasing. We simply do not
need the first requirement as the target file may
not have been loaded yet by the time the local
binding is referenced, and we do not need the
second feature since use statement bindings are
local and cannot be used for imports across files.
Copy link
Contributor

@NeunEinser NeunEinser left a comment

Choose a reason for hiding this comment

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

I don't think I quite understand why alias didn't work. Is this a bug in how aliases are implemented or expected behavior when trying to declare an alias of sth not loaded yet?

If it is expected behavior, how would you use aliases in a way that avoids this problem?

All known edge cases are solved, NeunEinser:neun is updated.

packages/mcdoc/src/binder/index.ts Outdated Show resolved Hide resolved
@NeunEinser NeunEinser requested a review from misode May 25, 2024 13:05
@SPGoding
Copy link
Member Author

expected behavior when trying to declare an alias of sth not loaded yet

it's mostly this. i could still use an alias but don't actually check the existence of the target when resolving it, in which case using a custom data is just simpler than using an alias.

@TheAfroOfDoom TheAfroOfDoom removed their request for review May 25, 2024 18:33
Copy link
Member

@misode misode left a comment

Choose a reason for hiding this comment

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

The issues on Neun's branch seem to be resolved!

packages/core/src/symbol/SymbolUtil.ts Show resolved Hide resolved
@MulverineX MulverineX merged commit 6103be7 into main May 26, 2024
3 checks passed
@MulverineX MulverineX deleted the mcdoc-use-global-binding branch May 26, 2024 23:00
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.

References to use statements aren't being resolved properly by mcdoc yet
5 participants