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

Heaps regression w/dev #9911

Closed
ncannasse opened this issue Oct 15, 2020 · 11 comments
Closed

Heaps regression w/dev #9911

ncannasse opened this issue Oct 15, 2020 · 11 comments
Assignees
Milestone

Comments

@ncannasse
Copy link
Member

Not sure since then but there is a regression with latest dev wrt heaps:

https://travis-ci.org/github/HeapsIO/heaps/jobs/736018107

@nadako
Copy link
Member

nadako commented Oct 15, 2020

I had this locally as well when I was trying heaps with latest haxe, and I didn't understand this. Surely the expected type is an enum abstract type there. Maybe it gets lost somehow because slides.shader is a type parameter?

@RealyUniqueName
Copy link
Member

This happens because the setter for mode is not type-hinted. And because of a cast the argument type and the return type of set_mode cannot be inferred:

function set_mode(m) { smode = cast m; return m; }

https://github.com/HeapsIO/heaps/blob/06a72533183691e8857e52abde97e6c29229afab/h3d/shader/pbr/Slides.hx#L69-L69
AFAIR I did a change to infer getter/setter types based on the property type. That change was probably lost during recent monomorph changes.

@RealyUniqueName
Copy link
Member

Reduced:
Enm.hx

enum abstract Enm(String) {
	var Full;
}

Main.hx

class Main {
	static var x(never,set):Enm;
	static function set_x(v) return v;

	static function main() {
		x = Full;
	}
}

@RealyUniqueName
Copy link
Member

Bisected to 72e61e0

@RealyUniqueName
Copy link
Member

Based on @Simn's comment in that commit I don't think we can fix this as it would toggle another issue.
Given how simple this is to workaround by type-hinting either an argument or return type of a setter I'm closing this as wontfix.

@ncannasse
Copy link
Member Author

Should we not infer the setter argument to be of the type of the variable it binds ?
Would also allow completion in setter, without the need to type hint the variable.

@RealyUniqueName
Copy link
Member

I struggle to find a solution for this.
At the PConnectField build pass the type for setter is TLazy and we should not resolve it according to @Simn's comment I linked earlier. At any earlier build pass there's no reliable way to detect property-setter relations.
So, the only way I see is to patch type hints for setters before initializing tclass_field instances in init_class (here). But this just looks like a dirty hack.

@RealyUniqueName
Copy link
Member

RealyUniqueName commented Jan 20, 2021

Actually even hacking type-hints of setters in AST is not reliable because of overloading.
One could have a proper setter defined and an overloaded function without type hints, which would be unrelated to the property.

@RealyUniqueName RealyUniqueName modified the milestones: Release 4.2, Backlog Feb 2, 2021
@Simn Simn removed this from the Backlog milestone Mar 24, 2023
@Simn Simn self-assigned this Mar 24, 2023
@Simn Simn modified the milestones: Release 4.3, Later Mar 24, 2023
@Simn Simn removed the regression label Mar 29, 2023
@Simn
Copy link
Member

Simn commented Mar 29, 2023

Alex's example compiles on latest development, and nobody here mentioned how this was failing in the first place... I'll assume that this is not something urgent to look into.

@kLabz
Copy link
Contributor

kLabz commented Mar 29, 2023

Should we not infer the setter argument to be of the type of the variable it binds ?
Would also allow completion in setter, without the need to type hint the variable.

This reminds me of a change that was added to add some type inference to getters/setters, which I had to update because it was not checking if it was linked to an actual property.

So uh maybe this issue was actually addressed but not closed?

@Simn
Copy link
Member

Simn commented Mar 29, 2023

Ah yeah, it was probably #10569.

@Simn Simn closed this as completed Mar 29, 2023
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

5 participants