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

local variables should always win over helper #17121

Closed
chancancode opened this issue Oct 16, 2018 · 4 comments · Fixed by #17135
Closed

local variables should always win over helper #17121

chancancode opened this issue Oct 16, 2018 · 4 comments · Fixed by #17135

Comments

@chancancode
Copy link
Member

We tried to fix this in #14520, but apparently we didn't catch all the cases.

Reproduction: https://ember-twiddle.com/dcce0cd429ceb1d454d62988937bfd15?openFiles=templates.application.hbs%2C

Options are:

  1. just fix it
  2. error for one cycle then fix
  3. deprecate for one LTS cycle then fix
@rwjblue
Copy link
Member

rwjblue commented Oct 16, 2018

IMHO, this is clearly a bug and we should just fix it (Option 1).

@chancancode
Copy link
Member Author

imo, these are also bugs but in the sub-expression positions, but that's probably more controversial: https://ember-twiddle.com/95790479cef1d910bdc12ffcfc446149?openFiles=templates.application.hbs%2C (cc @wycats @mmun)

@chancancode
Copy link
Member Author

Not sure if the approach is still good for the current glimmer codebase, but here was the last patch that fixed the {{foo}} cases: glimmerjs/glimmer-vm#335

@mmun
Copy link
Member

mmun commented Oct 18, 2018

👍 from me. I agree with all the statements in the second twiddle.

chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{concat (foo)}}
            ~~~ shadowed helper invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the precense of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This is now an error (assertion).

This paves the way for allowing "contextual helpers" in the future, where
the local variable `foo` contains an invocable helper value.

Partially addresses #17121, allowing the rest of the bugs to be fixed in
Glimmer VM.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{concat (foo)}}
            ~~~ shadowed helper invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This is now an error (assertion).

This paves the way for allowing "contextual helpers" in the future, where
the local variable `foo` contains an invocable helper value.

Partially addresses #17121, allowing the rest of the bugs to be fixed in
Glimmer VM.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{concat (foo)}}
            ~~~ shadowed helper invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This is now an error (assertion).

This paves the way for allowing "contextual helpers" in the future, where
the local variable `foo` contains an invocable helper value.

Partially addresses #17121, allowing the rest of the bugs to be fixed in
Glimmer VM.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{concat (foo)}}
            ~~~ shadowed helper invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This is now an error (assertion).

This paves the way for allowing "contextual helpers" in the future, where
the local variable `foo` contains an invocable helper value.

Partially addresses #17121, allowing the rest of the bugs to be fixed in
Glimmer VM.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{concat (foo)}}
            ~~~ shadowed helper invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This is now an error (assertion).

This paves the way for allowing "contextual helpers" in the future, where
the local variable `foo` contains an invocable helper value.

Partially addresses #17121, allowing the rest of the bugs to be fixed in
Glimmer VM.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{foo 1 2 3}}
    ~~~ local variable invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This will now invoke `foo` local
variable as a contextual component.

(In other words, this removes the remaining "dot rule" exceptions for
contextual components, as per [RFC #311](https://github.com/emberjs/rfcs/blob/master/text/0311-angle-bracket-invocation.md).)

This commit also fixed another related issue:

```hbs
{{#let (hash foo="foo-bar") as |h|}}
  {{h.foo 1 2 3}}
    ~~~~~ this is a string, not a component!
{{/let}}
```

Previously, this would have tried to resolve and invoke the `foo-bar`
component. This is an unintended consequence (i.e. a bug) of the "dot
rule" implementation. This will now raise a `TypeError` in development
mode and result in undefined behavior in production builds (probably
some other runtime error deep inside the Glimmer VM internals).

Fixes #17121.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{foo 1 2 3}}
    ~~~ local variable invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This will now invoke `foo` local
variable as a contextual component.

(In other words, this removes the remaining "dot rule" exceptions for
contextual components, as per [RFC #311](https://github.com/emberjs/rfcs/blob/master/text/0311-angle-bracket-invocation.md).)

This commit also fixed another related issue:

```hbs
{{#let (hash foo="foo-bar") as |h|}}
  {{h.foo 1 2 3}}
    ~~~~~ this is a string, not a component!
{{/let}}
```

Previously, this would have tried to resolve and invoke the `foo-bar`
component. This is an unintended consequence (i.e. a bug) of the "dot
rule" implementation. This will now raise a `TypeError` in development
mode and result in undefined behavior in production builds (probably
some other runtime error deep inside the Glimmer VM internals).

Fixes #17121.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{concat (foo)}}
            ~~~ shadowed helper invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This is now an error (assertion).

This paves the way for allowing "contextual helpers" in the future, where
the local variable `foo` contains an invocable helper value.

Partially addresses #17121, allowing the rest of the bugs to be fixed in
Glimmer VM.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{foo 1 2 3}}
    ~~~ local variable invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This will now invoke `foo` local
variable as a contextual component.

In other words, this removes the remaining "dot rule" exceptions for
contextual components, as per [RFC #311](https://github.com/emberjs/rfcs/blob/master/text/0311-angle-bracket-invocation.md):

> We propose to relax that rule to match the proposed angle bracket
> invocation semantics (i.e. allowing local variables without a dot, as
> well as `@names`, but disallowing implicit `this` lookup).

This commit also fixed another related issue:

```hbs
{{#let (hash foo="foo-bar") as |h|}}
  {{h.foo 1 2 3}}
    ~~~~~ this is a string, not a component!
{{/let}}
```

Previously, this would have tried to resolve and invoke the `foo-bar`
component. This is an unintended consequence (i.e. a bug) of the "dot
rule" implementation. This will now raise a `TypeError` in development
mode and result in undefined behavior in production builds (probably
some other runtime error deep inside the Glimmer VM internals).

Fixes #17121.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{foo 1 2 3}}
    ~~~ local variable invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This will now invoke `foo` local
variable as a contextual component.

In other words, this removes the remaining "dot rule" exceptions for
contextual components, as per [RFC #311](https://github.com/emberjs/rfcs/blob/master/text/0311-angle-bracket-invocation.md):

> We propose to relax that rule to match the proposed angle bracket
> invocation semantics (i.e. allowing local variables without a dot, as
> well as `@names`, but disallowing implicit `this` lookup).

This commit also fixed another related issue:

```hbs
{{#let (hash foo="foo-bar") as |h|}}
  {{h.foo 1 2 3}}
    ~~~~~ this is a string, not a component!
{{/let}}
```

Previously, this would have tried to resolve and invoke the `foo-bar`
component. This is an unintended consequence (i.e. a bug) of the "dot
rule" implementation. This will now raise a `TypeError` in development
mode and result in undefined behavior in production builds (probably
some other runtime error deep inside the Glimmer VM internals).

Fixes #17121, #16510.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{concat (foo)}}
            ~~~ shadowed helper invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This is now an error (assertion).

This paves the way for allowing "contextual helpers" in the future, where
the local variable `foo` contains an invocable helper value.

Partially addresses #17121, allowing the rest of the bugs to be fixed in
Glimmer VM.
chancancode added a commit that referenced this issue Oct 19, 2018
```hbs
{{#let this.foo as |foo|}}
  {{foo 1 2 3}}
    ~~~ local variable invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This will now invoke `foo` local
variable as a contextual component.

In other words, this removes the remaining "dot rule" exceptions for
contextual components, as per [RFC #311](https://github.com/emberjs/rfcs/blob/master/text/0311-angle-bracket-invocation.md):

> We propose to relax that rule to match the proposed angle bracket
> invocation semantics (i.e. allowing local variables without a dot, as
> well as `@names`, but disallowing implicit `this` lookup).

This commit also fixed another related issue:

```hbs
{{#let (hash foo="foo-bar") as |h|}}
  {{h.foo 1 2 3}}
    ~~~~~ this is a string, not a component!
{{/let}}
```

Previously, this would have tried to resolve and invoke the `foo-bar`
component. This is an unintended consequence (i.e. a bug) of the "dot
rule" implementation. This will now raise a `TypeError` in development
mode and result in undefined behavior in production builds (probably
some other runtime error deep inside the Glimmer VM internals).

Fixes #17121, #16510.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants