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

BREAKING CHANGE: fix: detect the premature usage of @global variant #2624

Conversation

HerrCai0907
Copy link
Member

@HerrCai0907 HerrCai0907 commented Jan 19, 2023

fix #2622

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

@HerrCai0907
Copy link
Member Author

The bug will cause compile result is incorrect.

// index.ts
import {} from "./a";
import {} from "./b";

// a.ts
class T {
  a: i32;
}
let a = t;

// b.ts
class T {}
@global export const t = new T();
const b = new T();

the compile result is

 (func $start:assembly/a
  global.get $~lib/memory/__heap_base
  i32.const 4
  i32.add
  i32.const 15
  i32.add
  i32.const 15
  i32.const -1
  i32.xor
  i32.and
  i32.const 4
  i32.sub
  global.set $~lib/rt/stub/startOffset
  global.get $~lib/rt/stub/startOffset
  global.set $~lib/rt/stub/offset
  i32.const 0
  call $assembly/a/T#constructor
  global.set $assembly/b/t
  global.get $assembly/b/t
  global.set $assembly/a/a
 )

It use assign a/a and b/t with call $assembly/a/T#constructor but actually it should be call $assembly/b/T#constructor

@dcodeIO
Copy link
Member

dcodeIO commented Jan 19, 2023

Interesting find, hmm. Appears that the issue arises because, in this particular situation with a custom @global in an imported file, using a @global variable before its source file executes will compile it out-of-order, here before any other top-level statements in b.ts, which then is similar in nature to @lazy, sharing its drawbacks. One could almost say that this sort of usage is not intended, as it modifies compilation order as a side-effect which can, even with the fix, lead to subtle issues.

Hence I wonder if it would be preferable to detect this situation and raise an error that t is used too early, say before its declaration? The fix would then be to import b.ts before a.ts, instead of permitting potentially undesirable side-effects?

Example of such a side-effect:

// index.ts
import {} from "./a";
import {} from "./b";

// a.ts
t += 41;

// b.ts
@global export let t = 1;
const b = t; // on a glimpse one would expect b = 1, but is 42

Similarly:

// index.ts
import {} from "./a";
import {} from "./b";

// a.ts
console.log(t.toString()); // one could expect "42", but is "1"

// b.ts
@global export let t = 1;
t = 42;

@HerrCai0907 HerrCai0907 changed the title fix: global decorator causes compiler use incorrect flow BREAKING CHANGE: fix: detect the premature usage of @global variant Jan 20, 2023
@HerrCai0907
Copy link
Member Author

Hence I wonder if it would be preferable to detect this situation and raise an error that t is used too early, say before its declaration? The fix would then be to import b.ts before a.ts, instead of permitting potentially undesirable side-effects?

I am wondering how to handle @lazy @global. I have no idea to detect this case because the @lazy decorator make the variant not compile until used. So maybe we can make an exception for this case.

@dcodeIO
Copy link
Member

dcodeIO commented Jan 20, 2023

If both @lazy and @global are present together, it seems OK to permit potential side-effects for now, given that @lazy permits it already. In practice, if both are present, the error would then not be diagnosed.

Comment on lines +7342 to +7349
if (!global.hasDecorator(DecoratorFlags.Lazy) && global.hasDecorator(DecoratorFlags.Global) && !global.is(CommonFlags.Compiled)) {
this.error(
DiagnosticCode.Variable_0_used_before_its_declaration,
expression.range,
global.internalName
);
return module.unreachable();
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this cover all possible cases? For example, if the global is not read (a = t1), but assigned (t1 = something), it looks like this branch alone would not suffice.

@dcodeIO dcodeIO mentioned this pull request Jan 23, 2023
2 tasks
@HerrCai0907
Copy link
Member Author

replaced by #2632

@HerrCai0907 HerrCai0907 deleted the fix/compile-global-use-incorrect-flow branch March 2, 2023 13:29
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.

@global causes compile error
2 participants