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

Inlining issue because @:coreApi is annoying #7379

Open
Simn opened this issue Sep 3, 2018 · 5 comments
Open

Inlining issue because @:coreApi is annoying #7379

Simn opened this issue Sep 3, 2018 · 5 comments

Comments

@Simn
Copy link
Member

Simn commented Sep 3, 2018

class Main {
	static public function main() {
		var map = new StringMap();
		map.set("foo", 12);
		map.set("bar", 13);
		for (value in map) { }
	}
}

This generates:

	var value = new haxe_ds__$StringMap_StringMapIterator(map,map.arrayKeys());
	while(value.hasNext()) value.next();

The reason for that is obvious in the dump file:

[Var value(4799):Iterator<Int>]
	[Cast:Iterator<Int>]
		[New:haxe.ds._StringMap.StringMapIterator<Int>]
			haxe.ds._StringMap.StringMapIterator<Int>
			[Local map(4563):haxe.ds.StringMap<Int>:haxe.ds.StringMap<Int>]
			[Call:Array<String>]
				[Field:Void -> Array<String>]
					[Local map(4563):haxe.ds.StringMap<Int>:haxe.ds.StringMap<Int>]
					[FInstance:Void -> Array<String>]
						haxe.ds.StringMap<Int>
						arrayKeys
[While:Void]
	[Parenthesis:Bool]
		[Call:Bool]
			[Field:Void -> Bool]
				[Local value(4799):Iterator<Int>:Iterator<Int>]
				[FAnon:Void -> Bool] hasNext
	[Call:Int]
		[Field:Void -> Int]
			[Local value(4799):Iterator<Int>:Iterator<Int>]
			[FAnon:Void -> Int] next

The underlying problem here is that StringMap.iterator is typed to be Iterator<T>, which means the next/hasNext field accesses are made on the structure and are not inlined.

From the inliner's point of view, this is the correct behavior. We don't want to lose the return type of an inline function, so casting to Iterator<Int> is only consistent.

I think @:coreApi should allow covariance for return types so that StringMap.iterator can be typed to return StringMapIterator<T> instead of Iterator<T>.

@Simn Simn added the regression label Sep 3, 2018
@Simn Simn added this to the Release 4.0 milestone Sep 3, 2018
@Simn Simn removed the regression label Sep 3, 2018
@Simn
Copy link
Member Author

Simn commented Sep 3, 2018

Surprisingly, this isn't a regression. But it's pretty dumb...

@ncannasse
Copy link
Member

covariance is fine, but requires proper docgen handling so it doesn't generate different method signatures

@Simn
Copy link
Member Author

Simn commented Sep 4, 2018

Yes docgen is also my main concern here...

@Simn
Copy link
Member Author

Simn commented Sep 5, 2018

Supporting covariance here is not an easy change. We currently use type_eq which has special semantics for the EqCoreType case. That's because we compare classes from different contexts, which means cTarget != cCoreType. Supporting covariance would require us to go through unify instead, but unify doesn't support this kind of equality checks.

@Simn Simn modified the milestones: Release 4.0, Backlog Sep 5, 2018
@Simn
Copy link
Member Author

Simn commented Sep 5, 2018

Not a regression and very tricky, pushing back.

@RealyUniqueName RealyUniqueName changed the title Inline regression because @:coreApi is annoying Inlining issue because @:coreApi is annoying Sep 9, 2019
@Simn Simn modified the milestones: Backlog, Later Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants