Add "expand" command to Crystal tools #3732

makenowjust · 2016-12-19T13:24:35Z

crystal tool expand expands macro for given location.

See asciinemas:

bcardiff · 2016-12-19T14:24:42Z

Nice. I would like to see some specs. At least some basic one that will ensure some scenarios wont fail: nested classes with modules, top level expansions.

I bet you wish there was some kind of index per file/line so you don't need to lookup the whole hierarchy of types. For now we are good, but in the future we might need to enhance de compiler with some of those indices i think.

bcardiff · 2016-12-19T14:29:42Z

Regarding the specs one that will result in multiple expansions should be interesting.

And json support with an array of the different levels of expansions. So we can zoom in/out. WDYT?

makenowjust · 2016-12-19T14:43:58Z

@bcardiff

And json support with an array of the different levels of expansions. So we can zoom in/out. WDYT?

When macros are nested, final expansion result may be too complex. In this case it is useful.

Regarding the specs one that will result in multiple expansions should be interesting.

If there is a macro in a method definition and the method definition has multiple type signatures, the macro has multiple expansion. For example:

macro foo
  puts "hello"
end

def bar(x)
  foo
end

bar 10
bar "str"

./bin/crystal tool expand -c foo.cr:6:3 foo.cr yields:

2 expansions found
expansion 1:
   foo

~> puts("hello")

expansion 2:
   foo

~> puts("hello")

I'll try to write specs. But, it is midnight in Japan, I am going to bed. Good night!

makenowjust · 2016-12-19T14:46:37Z

tiny .vimrc for demos:

command! -buffer -nargs=0 CrystalExpand echo s:crystal_expand(expand('%'), getpos('.'))

function! s:crystal_expand(file, pos)
  let l:cmd = printf('./bin/crystal tool expand --no-color -c %s:%d:%d %s', a:file, a:pos[1], a:pos[2], a:file)
  return system(l:cmd)
endfunction

RX14 · 2016-12-19T15:46:06Z

Could we run the output through crystal format? Some of the generated macro code isn't particularly beautiful.

asterite · 2016-12-20T11:47:15Z

@makenowjust Thank you for this! I think this tool can be useful, I just need to think what's the criteria to merge new tools.

Regardless of that, I find it really cool that you are hacking the compiler :-)

bcardiff · 2016-12-21T15:04:21Z

src/compiler/crystal/tools/context.cr

        # in which the (start) location might be in one file and the end location in another.
-        @target_location.between?(loc_start, loc_end) || loc_start.filename != loc_end.filename
+        @target_location.between?(loc_start, loc_end) || (!@in_defs && loc_start.filename != loc_end.filename)
+        # When node generated by macro and macro has yielding block, it also makes `loc_start.filename != loc_end.filename` true.


Great! Can a spec be added also for this case? The specs are at ./spec/compiler/crystal/tools/context_spec.cr and should be easy to add a sample. There are some unicode chars that act as user cursor so you can easily add specs.

It is just ad-hoc solution. The radical problem is yield in macro expansion sets its location, but doesn't reset it. Now, I have an idea for it. I'll make a new pull request to solve it and I am going to revert this commit after this pull request is merged.

It is fixed in #3751. I'll revert and rebase.

makenowjust · 2016-12-26T11:44:26Z

@bcardiff Added specs just now!

bcardiff

@makenowjust sorry for the delay. I've made some comments. Some are request for changes.

Would you be able to apply them and rebase this PR?

bcardiff · 2017-01-11T13:43:59Z

spec/compiler/crystal/tools/expand_spec.cr

+    {% end %}
+    CODE
+
+    original = <<-CODE


Many specs of assert_expand_result_simple could be shortened with a variation: assert_expand_single ? That could strip the line ‸ to get the original. I count 11 occurrences that could be changed with this. WDYT?

bcardiff · 2017-01-11T13:54:41Z

src/compiler/crystal/command.cr

@@ -393,6 +397,11 @@ class Crystal::Command
      end
    end

+    if expand


rather than adding an expand param to create_compiler and compile_no_codegen. I say to promote no_cleanup (default false) as param in this two methods.

And add no_cleanup and wants_doc to cursor_command.

The create_compiler needs hierarchy/cursor_command to know which command line options to parse. Since the expand is a cursor_command I think removing expand param make sense.

bcardiff · 2017-01-11T14:01:04Z

src/compiler/crystal/tools/expand.cr

+  class ExpandTrace
+    JSON.mapping({
+      original:  {type: String},
+      expandeds: {type: Array(String)},


I don't think expandeds is a word. I would suggest expansions or progressive_expansions.

~~Yes. I think expansions is better.~~

Hmm, but we will get expansions.expansions. It is very wrong.

expanded is also used to address a plural quantity, so you can just s/expandeds/expanded :)

bcardiff · 2017-01-11T14:31:12Z

src/compiler/crystal/tools/expand.cr

+    def self.ast_to_s(node)
+      s = String.build { |io| node.to_s(io, emit_doc: true) }
+      return s unless node.is_a?(MacroIf) || node.is_a?(MacroFor)
+      indent = node.location.not_nil!.column_number - 1


The case for changing the indentation is not stressed in the specs. If the following code that split/join the lines is removed all specs passed. Some indented if & for macros are needed in the specs.

Note: I don't fully follow why this is needed. Could the node.to_s output a nice indentation directly. This logic could be refactor later to the ToSVisitor.

MacroIf and MacroFor have MacroBody, it is just sub-string of source text. For example:

begin {% if 1 == 1 %} hello {% end %} end

MacroIf#to_s result of {% if 1 == 1 %}...{% end %} is:

{% if 1 == 1 %} hello {% end %}

So, re-indentation is needed. I'll add specs for it.

bcardiff · 2017-01-11T14:35:16Z

spec/compiler/crystal/tools/expand_spec.cr

+  it "expands macro control {% if %}" do
+    code = <<-CODE
+    {% if 1 == 1 %}
+      ‸


In the other specs that use ‸ it is used in the middle of the code. Like: {% ‸if 1 == 1 %} in this case.
It is stripped before analyzing the code.

It could stay this way if you find it more comfortable, but it is a slight inconsistency.

"crystal tool expand" expands macros for given location. https://asciinema.org/a/96590

makenowjust · 2017-01-12T04:57:49Z

@bcardiff Updated.

bcardiff · 2017-01-13T15:15:34Z

🎉 🔨 🔧 Thanks again @makenowjust 🔧 🔨 🎉

makenowjust force-pushed the feature/crystal-tool/expand branch from 5f9bc34 to 3daafb3 Compare December 21, 2016 14:16

bcardiff reviewed Dec 21, 2016

View reviewed changes

makenowjust force-pushed the feature/crystal-tool/expand branch from 3daafb3 to 6e0b362 Compare December 23, 2016 13:01

makenowjust force-pushed the feature/crystal-tool/expand branch 2 times, most recently from 5abf7a4 to 9dfcf86 Compare December 27, 2016 02:04

bcardiff self-assigned this Jan 10, 2017

bcardiff requested changes Jan 11, 2017

View reviewed changes

Implement "crystal tool expand" command

14d2eae

"crystal tool expand" expands macros for given location. https://asciinema.org/a/96590

makenowjust force-pushed the feature/crystal-tool/expand branch from ac95b39 to 14d2eae Compare January 12, 2017 04:56

mverzilli added community:in-progress kind:feature pr:wip status:accepted labels Jan 12, 2017

bcardiff approved these changes Jan 13, 2017

View reviewed changes

bcardiff removed the pr:wip label Jan 13, 2017

bcardiff merged commit 6bd4bed into crystal-lang:master Jan 13, 2017

bcardiff added this to the Next milestone Jan 13, 2017

makenowjust deleted the feature/crystal-tool/expand branch January 13, 2017 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "expand" command to Crystal tools #3732

Add "expand" command to Crystal tools #3732

makenowjust commented Dec 19, 2016

bcardiff commented Dec 19, 2016

bcardiff commented Dec 19, 2016

makenowjust commented Dec 19, 2016 •

edited

makenowjust commented Dec 19, 2016

RX14 commented Dec 19, 2016

asterite commented Dec 20, 2016

bcardiff Dec 21, 2016

makenowjust Dec 21, 2016

makenowjust Dec 23, 2016

makenowjust commented Dec 26, 2016

bcardiff left a comment

bcardiff Jan 11, 2017

bcardiff Jan 11, 2017

bcardiff Jan 11, 2017

makenowjust Jan 12, 2017 •

edited

makenowjust Jan 12, 2017

mverzilli Jan 12, 2017

bcardiff Jan 11, 2017

makenowjust Jan 12, 2017

bcardiff Jan 11, 2017

makenowjust commented Jan 12, 2017

bcardiff commented Jan 13, 2017

Add "expand" command to Crystal tools #3732

Add "expand" command to Crystal tools #3732

Conversation

makenowjust commented Dec 19, 2016

bcardiff commented Dec 19, 2016

bcardiff commented Dec 19, 2016

makenowjust commented Dec 19, 2016 • edited

makenowjust commented Dec 19, 2016

RX14 commented Dec 19, 2016

asterite commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makenowjust commented Dec 26, 2016

bcardiff left a 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

makenowjust Jan 12, 2017 • edited

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

makenowjust commented Jan 12, 2017

bcardiff commented Jan 13, 2017

makenowjust commented Dec 19, 2016 •

edited

makenowjust Jan 12, 2017 •

edited