Implement the Trace AST node #2089

xzyfer · 2016-05-19T14:13:58Z

This PR is the combination of #2085 and #2086.
They have their own detailed descriptions I wont repeat here.

Unfortunately those two PRs each have a single failing spec
which is due to a missing implementation provided by the
alternative PR. Those PRs have been marked as DNM and
will closed by this PR. I'm doing this so that master stays green
in case a revert is required.

Spec sass/sass-spec#868
Closes #2085
Closes #2086
Fixes #1585

In order to fix sass#1585 we need to introduce the `Trace` node from Ruby Sass. This node interacts with propsets in an interesting way. Have two node to represent the single Ruby `Prop` node caused a lot of trouble with correct implementing check nesting logic for `Trace`. The trouble with implementing `Trace` were exaggerated by us incorrectly eval propsets into declarations in the expand step. This PR moves the de-nesting of prop nodes the cssize step to [better match Ruby Sass][1]. Later we should rename this node `Prop` but it's out of scope of this PR. This should allow my work on porting the `Trace` node over from Ruby Sass to fix sass#1585. [1]: https://github.com/sass/sass/blob/63586f1e51c0aec47a7afcc8cd17aa03f32f0a29/lib/sass/tree/visitors/cssize.rb#L149-L166

coveralls · 2016-05-19T14:34:58Z

Coverage decreased (-0.2%) to 79.066% when pulling 3b72dbc on xzyfer:feat/make-it-rain into 6e4409e on sass:master.

mgreter · 2016-05-19T14:35:10Z

Overall: LGTM.

From what I see in the code, the Trace AST is pretty tightly coupled to mixins, so maybe TraceMixin would be a bit more clear about that. And it might make sense to only store the pointer to the mixin c instead of a string from c->name()?

I will add some additional minor nitpicking directly to the code in this PR ...

mgreter · 2016-05-19T14:35:25Z

src/check_nesting.cpp

+  // }
+
+  bool CheckNesting::is_invalid_mixin_definition_parent(Statement* parent)
+  {


has bool signature but only returns false?

There either return false or thrown error. The Ruby Sass implement is return the error string or false which is gross in C++.

ACK. But wouldn't it be more sane to just make them void then? Not really a big deal, but signature just indicates something it is not actually doing this way.

I think void might actually make the usage nicer.

Agree. It probably would make sense to just name it "check_mixin_definition_parent" etc. That would also make it more clear that they actually do throw an error (I don't really expect that from something prefixed "is_").

Made them void and removed the is_ prefix. This makes the method names the same as Ruby Sass.

xzyfer · 2016-05-19T14:42:36Z

so maybe TraceMixin would be a bit more clear about that

I decided to keep it Trace because that's the Ruby name. I would also like to come back to renaming Declaration to Prop. It helps if we're all speaking the same language.

And it might make sense to only store the pointer to the mixin

The implement doesn't care about the mixin itself, just it's name and pstate. I presume the name for displaying error messages somewhere I haven't seen yet.

mgreter · 2016-05-19T14:53:40Z

@xzyfer so the name is actually not used so far? Leaving the Trace to basically be a Block node with some special meaning (as in being a result of a mixing call)?

mgreter · 2016-05-19T14:58:19Z

src/expand.cpp

@@ -98,6 +100,8 @@ namespace Sass {
      return k;
    }

+    this->at_root_without_rule = false;


On a third look I don't see why it shouldn't work here if you add the LOCAL_FLAG call here ... Since you seem to reset it just right before you exit the function (and therefore this variable scope).

Sorry, you're correct.

xzyfer · 2016-05-19T14:58:31Z

@mgreter I explain in more detail in #2086

The Trace node is a simple wrapper around the result of a mixin call
and @content. It exists as a way to group the resulting properties
as a block but maintain some metadata about the initiating mixin call
or @content block.

It also plays an important rule is bubble those blocks with @at-root
and nested property sets. This required implementing the final
missing cssize methods. As a result we should a complete @at-root
implementation.

mgreter · 2016-05-19T15:00:10Z

@xzyfer I fail to see what the "metadata" is ... maybe you can elaborate a bit more on that.

xzyfer · 2016-05-19T15:14:47Z

I'm not 100% sure myself. As far as I can tell the Trace is a block
around the resulting output of a mixin call. Instead of appending
the individual resulting nodes to the parent block, the Trace block
is appended. The Trace node keeps a reference to the pstate of
the mixin that created and it's name.

The primary usecase appears to be in maintaining a backtrace in
case the resulting nodes end up in a location that's invalid. FWIW
I don't believe I'm correctly handling the backtrace in this PR. Since
it's not covered by specs it's a manual process.

This is the explanation I got from @nex3

it allows us to reconstruct a stack trace that includes mixin invocations even after mixins have been resolved
it wraps the output of a mixin invocation and remembers that mixin's name and line/filename information
so that if, later on, we run into an error in that output
we can tell the user "this was caused by including this mixin"

The Trace node is a simple wrapper around the result of a mixin call and `@content`. It exists as a way to group the resulting properties as a block but maintain some metadata about the initiating mixin call or `@content` block. It also plays an important rule is bubble those blocks with `@at-root` and nested property sets. This required implementing the final missing `cssize` methods. As a result we should a complete `@at-root` implementation. `Trace` "wrapping" other nodes didn't play nice with the naive check nesting implementation I lazily wrote so I also implemented 99% of the check nesting visitor like-for-like with Ruby Sass. The missing piece is handling `@imports` which is the only part I didn't need to touch in this work. Spec sass/sass-spec#868 Fixes sass#1585

mgreter · 2016-05-19T15:20:52Z

OK, so it is pretty much the same thing I need for exposing import stack on C-API. After imports are resolved (in eval) we have the full AST Tree with Function_Call and once that call is evaluated, the context from where these calls were imported is lost (since it is not preserved in the AST Tree). So the easiest approach that came to my mind was to add an AST Node just to preserve that information, which is basically the same as here from what I get from your explanation ...

xzyfer · 2016-05-19T15:23:49Z

That's my understanding. The node was originally called MixinTraceNode in Ruby Sass but was renamed to TraceNode but is still so far only used for mixins.

coveralls · 2016-05-19T15:24:02Z

Coverage decreased (-0.1%) to 79.128% when pulling c15f22f on xzyfer:feat/make-it-rain into 6e4409e on sass:master.

mgreter · 2016-05-19T15:27:23Z

OK, that clears some things up. I will see if I can re-use the Trace Node for what I need with imports. BTW I'm aware of the situation with the various StackTraces. We have something in Env and also various stack objects in various phases. I think we should refactor and unify all these cases in one smart class that can be easily handled and queried. But I think we first need to add the traces feature for the imports and (custom) functions. This work should also include the --trace cli option then.

xzyfer · 2016-05-19T15:32:05Z

I will see if I can re-use the Trace Node for what I need with imports

I would avoid this. Ruby Sass is explicit about it's usecase.

A solely static node left over after a mixin include or @content has been performed.
Its sole purpose is to wrap exceptions to add to the backtrace.
Source

There are various assumption made in places like check nesting and cssize visitors around this.

coveralls · 2016-05-19T15:34:50Z

Coverage increased (+0.2%) to 79.395% when pulling c15f22f on xzyfer:feat/make-it-rain into 6e4409e on sass:master.

The Trace node is a simple wrapper around the result of a mixin call and `@content`. It exists as a way to group the resulting properties as a block but maintain some metadata about the initiating mixin call or `@content` block. It also plays an important rule is bubble those blocks with `@at-root` and nested property sets. This required implementing the final missing `cssize` methods. As a result we should a complete `@at-root` implementation. `Trace` "wrapping" other nodes didn't play nice with the naive check nesting implementation I lazily wrote so I also implemented 99% of the check nesting visitor like-for-like with Ruby Sass. The missing piece is handling `@imports` which is the only part I didn't need to touch in this work. Spec sass/sass-spec#868 Fixes sass#1585

coveralls · 2016-05-19T15:41:37Z

Coverage increased (+0.1%) to 79.392% when pulling 6f80638 on xzyfer:feat/make-it-rain into 6e4409e on sass:master.

xzyfer · 2016-05-19T15:44:02Z

I addressed the feedback, and did some additional clean up.
It's late here so I'm not going to wait up for CI.
I'll ship this in the morning.

xzyfer · 2016-05-19T15:45:21Z

I've also made sure the respective PRs this references are up to date with the changes made here so they serve as an accurate reference to the intentions of this merge.

coveralls · 2016-05-19T16:09:02Z

Coverage decreased (-0.1%) to 79.12% when pulling 634e74c on xzyfer:feat/make-it-rain into 6e4409e on sass:master.

mgreter · 2016-05-20T23:34:38Z

Nice work! Thx for that PR!

xzyfer added 2 commits May 20, 2016 00:02

Remove the Propset AST node

bb56f7b

xzyfer added Bug - Confirmed Dev - PR Ready labels May 19, 2016

xzyfer added this to the 3.3.7 milestone May 19, 2016

xzyfer self-assigned this May 19, 2016

mgreter reviewed May 19, 2016
View reviewed changes

xzyfer force-pushed the feat/make-it-rain branch from 3b72dbc to 8fef72b Compare May 19, 2016 15:17

xzyfer force-pushed the feat/make-it-rain branch from 8fef72b to c15f22f Compare May 19, 2016 15:17

xzyfer force-pushed the feat/make-it-rain branch from 04cf42a to 6f80638 Compare May 19, 2016 15:36

xzyfer merged commit ff4391f into sass:master May 20, 2016

xzyfer deleted the feat/make-it-rain branch May 20, 2016 23:14

This was referenced May 20, 2016

[WIP] POC of refactor to support at-root correctly #1900

Closed

mixin inside at-root not working correctly #2006

Closed

xzyfer mentioned this pull request Oct 1, 2016

@at-root regression with nested mixin #2198

Closed

xzyfer modified the milestones: 3.3.7, 3.4 Oct 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the Trace AST node #2089

Implement the Trace AST node #2089

xzyfer commented May 19, 2016

coveralls commented May 19, 2016 •

edited

Loading

mgreter commented May 19, 2016

mgreter May 19, 2016

xzyfer May 19, 2016

mgreter May 19, 2016

xzyfer May 19, 2016

mgreter May 19, 2016

xzyfer May 19, 2016

xzyfer commented May 19, 2016

mgreter commented May 19, 2016

mgreter May 19, 2016

xzyfer May 19, 2016

xzyfer commented May 19, 2016

mgreter commented May 19, 2016

xzyfer commented May 19, 2016

mgreter commented May 19, 2016 •

edited

Loading

xzyfer commented May 19, 2016

coveralls commented May 19, 2016 •

edited

Loading

mgreter commented May 19, 2016 •

edited

Loading

xzyfer commented May 19, 2016 •

edited

Loading

coveralls commented May 19, 2016

coveralls commented May 19, 2016

xzyfer commented May 19, 2016

xzyfer commented May 19, 2016

coveralls commented May 19, 2016 •

edited

Loading

mgreter commented May 20, 2016

Implement the Trace AST node #2089

Implement the Trace AST node #2089

Conversation

xzyfer commented May 19, 2016

coveralls commented May 19, 2016 • edited Loading

mgreter commented May 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzyfer commented May 19, 2016

mgreter commented May 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzyfer commented May 19, 2016

mgreter commented May 19, 2016

xzyfer commented May 19, 2016

mgreter commented May 19, 2016 • edited Loading

xzyfer commented May 19, 2016

coveralls commented May 19, 2016 • edited Loading

mgreter commented May 19, 2016 • edited Loading

xzyfer commented May 19, 2016 • edited Loading

coveralls commented May 19, 2016

coveralls commented May 19, 2016

xzyfer commented May 19, 2016

xzyfer commented May 19, 2016

coveralls commented May 19, 2016 • edited Loading

mgreter commented May 20, 2016

coveralls commented May 19, 2016 •

edited

Loading

mgreter commented May 19, 2016 •

edited

Loading

coveralls commented May 19, 2016 •

edited

Loading

mgreter commented May 19, 2016 •

edited

Loading

xzyfer commented May 19, 2016 •

edited

Loading

coveralls commented May 19, 2016 •

edited

Loading