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

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

Open
back2dos opened this issue Nov 22, 2023 · 6 comments
Open

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

back2dos opened this issue Nov 22, 2023 · 6 comments
Assignees

Comments

@back2dos
Copy link
Member

Original discussion about disallowing macro in macro: #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
Copy link
Member

Simn commented Nov 22, 2023

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 Simn self-assigned this Nov 22, 2023
@Simn
Copy link
Member

Simn commented Nov 22, 2023

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.

@Simn Simn closed this as completed in 297353c Nov 22, 2023
@back2dos
Copy link
Member Author

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
Copy link
Member

Simn commented Nov 22, 2023

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.

@Simn Simn reopened this Nov 22, 2023
@back2dos
Copy link
Member Author

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
Copy link
Member

Simn commented Nov 22, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants