fix(semantic): reference flags not correctly resolved when after an export stmt

[camc314]

fixes #7879 (comment)

TLDR is curently here:

oxc/crates/oxc_semantic/src/builder.rs

Lines 2130 to 2135 in cdd121b

	if self.current_reference_flags.is_empty() {
	ReferenceFlags::Read
	} else {
	// Take the current reference flags so that we can reset it to empty
	mem::take(&mut self.current_reference_flags)
	}

self.current_reference_flags.is_empty() is not empty, which causes the current ref flags to be used (this is incorrect, we should be using a fresh version of reference flags). Buy setting ref flags to None on exit export node, this issue is avoided

export BEFORE the reference ( incorrect reference flags)

https://playground.oxc.rs/#eNpVjjuOwzAMRK8isElj7A/YxtttkVOkkR3aECCJBskkdgzdPZISG0ilGc3DcFbooQUXJmI1q/m3bJIZmII5fHx2lg9/p4hzTXWZsCL3N+RekB3qvRUxRyKDs2I8S61cEzRA0K7Al1geWaLaGVrlCzbgXdRNS08T7mYJHfnNKdsoA3GAdrBeMDUwWRbk3Jh1adn0jtYPUMsj5hOA8vP1/QuZ6OmMI5Yx2QQX3eCebLBx9K8FlYvK5I+ebiW9InckOX4uSOkBCrdvkw==

export AFTER the reference ( correct reference flags)

https://playground.oxc.rs/#eNpVjj2uwjAQhK9ibZMmen/Sa0JHwSlonLCJLNm70a6BhMh3xzEEQeUZz6fZWaCDBlwYWaJZzN6KSaYXDqb6+m6tVLsjHQmnknfeqpoDs8EpIp208Et6Q+I8Yum5ffTcqh3UwNAsIGdaH50p2gmaKGeswTuKm9aOR3yZObTsNxfFkvYsAZreesVUw2hFUXJj1mvLpl9o+YBoZcB8AlD/fn7/IRMdn3DAdUw2wZHr3YMNlgb/XFA4isL+4Pm6pheUljXHjwUp3QEtmnBd

this PR fixes this issue by resetting the reference flags after exising an export stmt

[camc314]

• fix(semantic): reference flags not correctly resolved when after an export stmt #8134 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codspeed-hq]

CodSpeed Performance Report

Merging #8134 will not alter performance

_{Comparing c/12-26-fix_semantic_reference_flags_not_correctly_resolved_when_after_an_export_stmt (79af100) with main (74572de)}

Summary

✅ 29 untouched benchmarks

[Dunqing]

Thank you for working on this!

self.current_reference_flags.is_empty() is not empty, which causes the current ref flags to be used (this is incorrect, we should be using a fresh version of reference flags). Buy setting ref flags to None on exit export node, this issue is avoided

This is not the root cause, after resolving reference flags, the flags should be empty because it took, so the problem is why it still has ReferenceFlags::Type. After debugging, I found the problem is related to export if has source, you can see playground, and then I look through the AST, just found that if there is a source in export, all ExportSpeicifer are IdentifierName than IdentifierReference, so it cannot reset.

How to solve?

We should not set current reference flags if the export has source, and call visit_export_specifiers instead. The code looks like:

-        for specifier in &it.specifiers {
-            // `export type { a }` or `export { type a }` -> `a` is a type reference
-            if it.export_kind.is_type() || specifier.export_kind.is_type() {
-                self.current_reference_flags = ReferenceFlags::Type;
-            } else {
-                // If the export specifier is not a explicit type export, we consider it as a potential
-                // type and value reference. If it references to a value in the end, we would delete the
-                // `ReferenceFlags::Type` flag in `fn resolve_references_for_current_scope`.
-                self.current_reference_flags = ReferenceFlags::Read | ReferenceFlags::Type;
-            }
-            self.visit_export_specifier(specifier);
-        }
--
         if let Some(source) = &it.source {
             self.visit_string_literal(source);
+            self.visit_export_specifiers(&it.specifiers);
+        } else {
+            for specifier in &it.specifiers {
+                // `export type { a }` or `export { type a }` -> `a` is a type reference
+                if it.export_kind.is_type() || specifier.export_kind.is_type() {
+                    self.current_reference_flags = ReferenceFlags::Type;
+                } else {
+                    // If the export specifier is not a explicit type export, we conside
r it as a potential
+                    // type and value reference. If it references to a value in the end,
 we would delete the
+                    // `ReferenceFlags::Type` flag in `fn resolve_references_for_current
_scope`.
+                    self.current_reference_flags = ReferenceFlags::Read | ReferenceFlags
::Type;
+                }
+                self.visit_export_specifier(specifier);
+            }
         }

Feel free to make it better!

[camc314]

ohhh @Dunqing this makes so much sense, thanks for taking the time to help investigate this.
but wow this is complex.

So basically what was happening is:

1. semantic was looping over all export specifiers
2. it was then setting current_reference_flags
3. however as these were re-exports (IdentifierName vs IdentifierReference), semantic did not push this in as a reference
4. this meant that stale current_reference_flags ended up hanging around for the next time an ident reference was found.

Dunqing fixes this by:

1. only setting self.current_reference_flags if source is none (when source is non, the export specifiers must be IdentifierReferences over IdentifierNames
2. this means that the value current_refernce_flags is taken, replacing it with empty flags.
3. this means that the next identifierReference has the correct flags 🎉

[Dunqing]

ohhh @Dunqing this makes so much sense, thanks for taking the time to help investigate this. but wow this is complex.

Yes, it is complex and unintuitive, but I haven't found a way to make it better, If you are interested in finding a way to sort it out, do it!

So basically what was happening is:

1. semantic was looping over all export specifiers
2. it was then setting current_reference_flags
3. however as these were re-exports (IdentifierName vs IdentifierReference), semantic did not push this in as a reference
4. this meant that stale current_reference_flags ended up hanging around for the next time an ident reference was found.

Dunqing fixes this by:

1. only setting self.current_reference_flags if source is none (when source is non, the export specifiers must be IdentifierReferences over IdentifierNames
2. this means that the value current_refernce_flags is taken, replacing it with empty flags.
3. this means that the next identifierReference has the correct flags 🎉

Exactly, Thank you for summing it up!

[Boshen]

Merge activity

• Dec 27, 9:19 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Dec 27, 9:19 AM EST: A user added this pull request to the Graphite merge queue.
• Dec 27, 9:19 AM EST: A user removed this pull request from the Graphite merge queue.
• Dec 27, 9:19 AM EST: A user added this pull request to the Graphite merge queue.
• Dec 27, 9:19 AM EST: The merge label '0-merge' was removed. This PR will no longer be merged by the Graphite merge queue
• Dec 27, 9:24 AM EST: A user merged this pull request with the Graphite merge queue.