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

Agree and document the meaning of ReferenceFlags::Read and Write #5165

Closed
overlookmotel opened this issue Aug 24, 2024 · 10 comments · Fixed by #7368
Closed

Agree and document the meaning of ReferenceFlags::Read and Write #5165

overlookmotel opened this issue Aug 24, 2024 · 10 comments · Fixed by #7368
Assignees
Labels
A-semantic Area - Semantic

Comments

@overlookmotel
Copy link
Contributor

This question came up previously in #3326, and has reared its head again in #5158.

++x currently gets flagged as Write only.

This doesn't make sense to me as it is equivalent to x += 1 which is Read | Write.

The tests indicate this is intentional, but the reason why is not documented anywhere.

@Dunqing's guess was "I believe this variable can be safely removed by the Minifier/Compressor."

But as I pointed out, ++x can have side effects if x is an object with a valueOf method:

let x = {
    valueOf() { console.log("side effect!"); }
};

++x; // Logs "side effect!"

So that suggests that ++x cannot be removed by minifier in all cases.

Whatever the rationale is, can we please hash it out and document it?

@overlookmotel overlookmotel added the C-bug Category - Bug label Aug 24, 2024
@overlookmotel overlookmotel added A-semantic Area - Semantic and removed C-bug Category - Bug labels Aug 25, 2024
@Boshen Boshen self-assigned this Aug 29, 2024
@magic-akari
Copy link
Contributor

I have a simple idea.
To determine if an expression should be flagged as Read or Write, replace that expression with x.value:

const x =  {
    get value() {
        console.log('read');
    },
    set value(v) {
        console.log('write');
    }
}

For example, with const a = b = 1, we want to know the situation of b, so
const a = x.value = 1, and it finally prints 'write', so b gets flagged asWrite only.

@overlookmotel
Copy link
Contributor Author

I like that - it's easy to understand.

So by that definition of "read" and "write", ++x is Read | Write because this logs "read" and "write":

let x =  {
    get value() { console.log('read'); },
    set value(v) { console.log('write'); },
};
++x.value;

But... I am still wondering what the purpose of the read and write flags is? What are they meant to be used for? That might affect what is the right mental model to have.

@Boshen
Copy link
Member

Boshen commented Sep 7, 2024

https://tc39.es/ecma262/#sec-prefix-increment-operator-runtime-semantics-evaluation

image

This is the semantics of read and write.

@Boshen Boshen closed this as completed Sep 7, 2024
@overlookmotel
Copy link
Contributor Author

overlookmotel commented Sep 9, 2024

So do I understand right that the flags we currently have on ++x (Write only) are wrong? Should be Read | Write?

Either way, am reopening this issue as the "action" I was requesting is that we document it.

@overlookmotel overlookmotel reopened this Sep 9, 2024
@Boshen
Copy link
Member

Boshen commented Sep 9, 2024

@overlookmotel overlookmotel reopened this 1 hour ago

You may need to update the issue title and description.

@overlookmotel overlookmotel changed the title What does ReferenceFlags::Read mean? Agree and document the meaning of ReferenceFlags::Read and Write Sep 9, 2024
@overlookmotel
Copy link
Contributor Author

Have changed the issue title. To (hopefully) conclude this, have I understood correctly from spec above that x in ++x should be Read | Write?

@Boshen
Copy link
Member

Boshen commented Sep 9, 2024

Have changed the issue title. To (hopefully) conclude this, have I understood correctly from spec above that x in ++x should be Read | Write?

yes

@overlookmotel
Copy link
Contributor Author

Thank you. I'll add some docs, and open an issue to fix flags on ++x.

@overlookmotel overlookmotel assigned overlookmotel and unassigned Boshen Sep 9, 2024
@magic-akari
Copy link
Contributor

swc-project/swc#9558 (comment)

This is an example where you should use ReferenceFlags::Read and Write to replace rvalue enum values without affecting others.
In SWC, marking is_lhs to check if the current expression is an lvalue or rvalue complicates reuse and increases error risk.
If you've already analyzed Read/Write, you can apply it directly.

@overlookmotel
Copy link
Contributor Author

overlookmotel commented Nov 20, 2024

For those of us who find the spec hard to follow, a runtime test function for determining read/write status of vars.

Its output aligns with the spec.

function test(code) {
    console.log(`\n> ${code}`);

    const proxy = new Proxy(Object.create(null), {
        has(target, propName) {
            if (typeof propName === 'symbol') return Reflect.has(target, propName);
            return true;
        },
        get(target, propName) {
            const value = Reflect.get(target, propName);
            if (typeof propName !== 'symbol') console.log(`read  ${propName}: ${value}`);
            return value;
        },
        set(target, propName, value) {
            const success = Reflect.set(target, propName, value);
            if (typeof propName !== 'symbol') console.log(`write ${propName}: ${value}`);
            return success;
        },
    });

    (0, eval)(`__proxy => { with (__proxy) { ${code} } }`)(proxy);
}
test('x = y = 1;');
// write y: 1
// write x: 1

test('x = 1; x++;');
// write x: 1
// read  x: 1
// write x: 2

@Boshen Boshen closed this as completed Nov 20, 2024
Boshen pushed a commit that referenced this issue Nov 21, 2024
Add more docs to `ReferenceFlags`, mainly to include the runtime test in #5165 (comment).
Boshen pushed a commit that referenced this issue Nov 21, 2024
Add more docs to `ReferenceFlags`, mainly to include the runtime test in #5165 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semantic Area - Semantic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants