QuickTypes.jl icon indicating copy to clipboard operation
QuickTypes.jl copied to clipboard

Add Revise tests

Open etpinard opened this issue 1 year ago • 5 comments

Start with some basic Revise tests, that currently pass on v1.9.1.

etpinard avatar Dec 15 '23 19:12 etpinard

then https://github.com/cstjean/QuickTypes.jl/pull/27/commits/b459c6a0279b2d66b02f0fba3e32b4e960c20e92 leads to

image

The culprit seems to be in https://github.com/cstjean/QuickTypes.jl/commit/2ed4967e966e7249043795423286c0ddc48730c4. Reverting it makes the test/revise.jl suite pass.

etpinard avatar Dec 15 '23 19:12 etpinard

Thank you for the bug report! 121bab4 makes the revise tests pass, at the cost of breaking some other tests. Revise doesn't seem to like __source__ interpolation.

Is Revise really broken on your side? Maybe the Revise test failures are just an artifact of the way the revise tests are performed, and it's the tests themselves that need updating.

I appreciate the test suite, and it was quite useful to me. That said, it would be a bit sad to add this kind of maintenance burden on all packages that use macros, for what is arguably a Revise failure (heroic as Revise is).

I think once this is fixed, I'd merge the test suite, but wouldn't hesitate to drop it if it becomes a burden.

cstjean avatar Dec 16 '23 18:12 cstjean

Maybe the Revise test failures are just an artifact of the way the revise tests are performed, and it's the tests themselves that need updating.

No, the tests are working as expected. We can replicate the

MethodError: Cannot `convert` an object of type Nothing to an object of type Symbol

errors in real life scenarios.

etpinard avatar Dec 18 '23 19:12 etpinard

MacroTools.@q seems to be part of the problem.

With

diff --git a/test/assets/within-macro1.jl b/test/assets/within-macro1.jl
index 36e468b..f639abd 100644
--- a/test/assets/within-macro1.jl
+++ b/test/assets/within-macro1.jl
@@ -3,7 +3,7 @@ using MacroTools: @q, @capture

 macro mymodel(def)
     @capture(def, model_(; params__))
-    esc(@q begin
+    esc(quote
             $QuickTypes.@qstruct_fp $model(; $(params...))
         end)
 end
diff --git a/test/assets/within-macro2.jl b/test/assets/within-macro2.jl
index 76da376..d27b4a6 100644
--- a/test/assets/within-macro2.jl
+++ b/test/assets/within-macro2.jl
@@ -3,7 +3,7 @@ using MacroTools: @q, @capture

 macro mymodel(def)
     @capture(def, model_(; params__))
-    esc(@q begin
+    esc(quote
             $QuickTypes.@qstruct_fp $model(; $(params...))
         end)
 end

the Revise error goes away.

etpinard avatar Dec 19 '23 16:12 etpinard

Yes, @q strips away all line numbers, which Revise doesn't seem to like. I would try:

res = esc(@q begin
             $QuickTypes.@qstruct_fp $model(; $(params...))
         end)
res.args[1].args[2] = __source__
return res

The problem with normal quote is that the line numbers point to the macro definition code, which isn't great. Eg. before my latest PR, @qstruct Foo(); @which Foo() would point to the QuickTypes code, whereas we'd like it to point to the module where @qstruct Foo() is written. That's what the @q + res.args[1].args[2] = __source__ combo achieves.

Come to think of it, maybe the @q isn't necessary... But still, I don't think it hurts. quote puts a lot of LineNumberNode everywhere, and almost none of them are desirable from my POV.

cstjean avatar Dec 19 '23 17:12 cstjean