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

[typer] better error messages when importing nonexistent subtypes / fields #10899

Merged
merged 8 commits into from
Dec 27, 2022

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented Dec 22, 2022

See #10897

Would also be nice to do the same when using fully qualified paths, but not sure how feasible that would be.

@Simn
Copy link
Member

Simn commented Dec 22, 2022

Are these tests supposed to fail? There's only one failure and that's because of a different range.

@kLabz
Copy link
Contributor Author

kLabz commented Dec 22, 2022

No, I did implement those. I didn't commit the fully qualified ones yet, need to clean those a bit

@kLabz
Copy link
Contributor Author

kLabz commented Dec 22, 2022

Are these tests supposed to fail? There's only one failure and that's because of a different range.

I added failing tests 659de5c for the two cases I'm not sure how to handle

projects/Issue10897/fullpath-module-field-fail.hxml
-FullpathModuleField.hx:2: characters 2-18 : Module ModuleFields does not define field baz (Suggestion: bar)
+FullpathModuleField.hx:2: characters 2-14 : Module ModuleFields does not define type ModuleFields

projects/Issue10897/fullpath-subtype-fail.hxml
-FullpathSubtype.hx:2: characters 8-17 : Module Types does not define type Baz (Suggestion: Bar)
+FullpathSubtype.hx:2: characters 8-13 : Module Types does not define type Types
+FullpathSubtype.hx:2: characters 8-13 : ... For function argument 'v'

@Simn
Copy link
Member

Simn commented Dec 27, 2022

Yeah the expression-level ones are quite difficult. This portion of code has been working very well since the nadako-rework, but the underlying logic is still complicated. There's a lot of try/resume going on, so communicating meaningful information is not very obvious.

I'll take a look but I don't want to spend too much time on this because it'd be little more than a small QOL improvement anyway.

@kLabz
Copy link
Contributor Author

kLabz commented Dec 27, 2022

Yeah I agree; handling only imports for now is fine I guess, it's already an improvement.
Can just disable/remove the expression level tests for now if it's any trouble to implement.

@Simn
Copy link
Member

Simn commented Dec 27, 2022

We can catch this case in handle_efield with this (next to the Unknown_ident one):

| Error (Type_not_found((_,mname),tname,Not_defined),_) when tname = first.name && tname = mname

However, when we put ModuleFields in a package and try pack.ModuleFields.baz, we already end up in another code path which results in Type not found: pack.ModuleFields.

So yeah I think we shouldn't worry about this at the moment and just merge the changes you've already made. It might be possible to clean up this field chain handling code some more (there are some TODO comments there as well), but as I said, this isn't exactly easy.

@kLabz kLabz marked this pull request as ready for review December 27, 2022 08:08
@Simn Simn merged commit eeb62ef into HaxeFoundation:development Dec 27, 2022
@skial skial mentioned this pull request Jan 4, 2023
1 task
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

Successfully merging this pull request may close these issues.

2 participants