Skip to content

Commit

Permalink
[eval,hl] fix catching value exceptions
Browse files Browse the repository at this point in the history 
Closes #11321
  • Loading branch information
@kLabz
kLabz committed Sep 28, 2023
1 parent 804e27e commit 269fef4
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/filters/exceptions.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ let catch_native ctx catches t p =
| [], None ->
catches_to_ifs ctx catches t p
| [], Some catch ->
catches_to_ifs ctx [catch] t p
catches_to_ifs ctx (catch :: catches) t p

This comment has been minimized.

Sign in to view

Copy link
@Simn

Simn Sep 28, 2023

Member

Hmm, this changes exception order, right? I think it should be possible to construct a case which prioritizes ValueException over another catch clause that appears before it.

This comment has been minimized.

Sign in to view

Copy link
@kLabz

kLabz Sep 28, 2023

Author Contributor

Hmm like this:

class Main {
	static function main() {
		try {
			throw "!";
		} catch(e:haxe.ValueException) {
			trace('ValueException');
		} catch(e:String) {
			trace('String');
		} catch(e) {
			trace('CatchAll');
		}
	}
}

Printing "String" instead of "ValueException" ?

This comment has been minimized.

Sign in to view

Copy link
@Simn

Simn Sep 28, 2023

Member

I was thinking of the other way around, catching String before ValueException. But that appears to still work so yeah, I guess it's fine.

This comment has been minimized.

Sign in to view

Copy link
@kLabz

kLabz Sep 28, 2023

Author Contributor

Above example actually goes into | _ -> below, so not exactly related.

This comment has been minimized.

Sign in to view

Copy link
@Simn

Simn Sep 28, 2023

Member

Oh, I thought that code path activated as soon as there is a ValueException catch.
| _ ->
catches_as_value_exception ctx handle_as_value_exception None t p
:: catches_to_ifs ctx catches t p
Expand Down
8 changes: 8 additions & 0 deletions tests/unit/src/unit/TestExceptions.hx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,14 @@ class TestExceptions extends Test {
} catch(e:String) {
eq('string', e);
}

try {
throw new CustomHaxeException('Terrible error');
} catch(e:haxe.ValueException) {
throw 'should not happen';
} catch(e) {
Assert.pass();
}
}

public function testCustomNativeException() {
Expand Down

0 comments on commit 269fef4

Please sign in to comment.