HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.14k stars 656 forks source link

The macro-in-macro check seems insufficient. #11403

Open back2dos opened 10 months ago

back2dos commented 10 months ago

Original discussion about disallowing macro in macro: https://github.com/HaxeFoundation/haxe/issues/7348

It seems the check only occurs when actually calling a macro from a macro:

class Test {
    static function main() {
        #if !macro
        for (v in getValues())
            trace(v);
        #end
    }
    public static macro function getValues() {
        getValues();// Uncaught exception macro-in-macro
        return macro [];
    }
}

But not when calling a macro in macro context:

class Test {
    static function main() {
        for (v in getValues()) // You can't iterate on a Dynamic value, please specify Iterator or Iterable
            trace(v);
    }
    public static macro function getValues() {
        return macro [];
    }
}

Does that really make sense?

Simn commented 10 months ago

This was an intentional change, the macro run-time now throws an exception if it encounters a macro call, and the typer doesn't care about it otherwise.

But yes, the way this errors isn't ideal in this situation. It would be better if any function that has a macro call in macro context would cancel its typing, and then emit a useful error if it ends up being called.

Simn commented 10 months ago

Although that might be a bit too simplistic too. I see no reason why code like this shouldn't work:

class Main {
    static function main() {}

    static function test(b:Bool) {
        if (b) {
            getValues();
        }
    }

    public static macro function getValues() {
        test(false);
        return macro [];
    }
}

For this particular issue, the solution might be to support for (v in throw "") {}, by ignoring the entire for-thing and just generating the throw "". It looks very dumb but that's really the cause of the problem here.

There might be other cases where the inserted throw could throw typing errors, but this should be somewhat rare.

back2dos commented 10 months ago

Thanks for the fix. But I wonder if this is the right approach, because the underlying issue can take more than one form (I suspect there are quite a few):

class Test {
    static function main() {
        switch getValues() {
            case [1]: // Unrecognized pattern: [1]
            default:
        }
    }

    public static macro function getValues() {
        return macro [1];
    }
}

I see no reason why code like this shouldn't work

I really don't see why it should. What is the use case here? Can there even be a use case for allowing code that MUST NOT run?

My original point was that it indeed shouldn't work. The only reliable effect of allowing this is to frustrate users. FWIW I really do think that when the typer sees a macro call in macro context it should error right there, much like it does with @:build.

Simn commented 10 months ago

Failing during typing is not practical, but perhaps it's indeed better to just fail if we're calling a function that has a macro in macro context.

I'm not sure though because any problem we run into here can also be reproduced by manually using throw "macro-in-macro". I feel like this should never fail because it's a control flow terminator, and we usually allow var a = 1 + return and all sorts of other nonsense. Will think about this some more.

back2dos commented 10 months ago

I'm sort of worried by the whole approach, because it allows amassing piles of invalid code. This compiles with the latest change:

class Test {
    static function main() {
        for (v in getValues()) {
            #if macro
            blarghfnth;// fnghhhtwrrt?
            #end
        }
    }

    public static macro function getValues() {
        return macro [1];
    }
}

Allowing unrunnable code to compile in macro context just because it happens not to run feels like embedding the worst possible subset of PHP straight into the compiler. The only thing that can reliably produce is frustration.

Moreover, there are cases that I'm not sure you can sensibly deal with: someMacro().match(Some(_)); is bound to fail with Unknown identifier : Some and someMacro().match(haxe.ds.Option.Some(_)); with the even more confusing Unknown identifier : _ . All the while the actual problem is a macro call in macro context, but there's no message to even hint at that.

Failing during typing is not practical

Are you saying it is not practical now? Or ever?

Because it it's the latter, I really just don't get why. Nothing good ever came from target context code unintentionally spilling into macro context. Even if it happens to compile at the time of writing, it's because the planetary alignment was just right. At best, it adds build time. At worst it produces giant piles of errors after a subsequent innocuous change. I don't think it's doing anyone a favor to actually support this.

Yes, failing during typing will probably mean that some code starts to break with "macro in macro" errors. If these get enough context, the code should be trivial to fix and will be better as a result.

Simn commented 10 months ago

I mean not practical ever. Even the very first macro example in the manual would no longer compile and require #if fencing, which is not acceptable. You're roughly 10 years too late with this request.

I guess not typing parts of the function at all wasn't a good idea. I'll see how we can get better failures for some of these more arcane situations.