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

Fix warnings for external declaration using unboxed types #1935

Merged
merged 5 commits into from Jul 31, 2018

Conversation

Projects
None yet
4 participants
@smuenzel-js
Copy link
Contributor

commented Jul 26, 2018

This fixes MPR#7828.
When we warn about unannotated and unboxable types used in external declarations, we were checking whether a type is unboxed, not whether it had an annotation.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

Could you convert typing-unboxed/test.ml to use expect tests? Right now the reference output is painful to read, and it makes it harder than necessary to review the PR.

Changes Outdated
@@ -260,6 +260,9 @@ Working version
(Florian Angeletti, report by Valentin Gatien-Baron,
review by Gabriel Scherer)

- MPR#7828: correct the conditions that generate warning 61,
Unboxable_type_in_prim_decl (Stefan Muenzel)

This comment has been minimized.

Copy link
@gasche

gasche Jul 27, 2018

Member

the credits should come on their own line, as in the other entries

@smuenzel-js

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

I've updated the tests for this patch to use expect tests and fixed the changes entry.

@gasche
Copy link
Member

left a comment

Would you mind also converting the existing tests in typing-unboxed/test.ml to expect tests? I know you are not responsible for writing those, but doing this conversion helps improve the testsuite, piece by piece, so it's good hygiene to grow expect tests around the parts of the compiler you touch.

Warning 61: This primitive declaration uses type i, which is unannotated and
unboxable. The representation of such types may change in future
versions. You should annotate the declaration of i with [@@boxed]
or [@@unboxed].

This comment has been minimized.

Copy link
@gasche

gasche Jul 27, 2018

Member

Why is this warning printed twice for the same location? Is it because it is printed once for each occurrence of i? Is it easy to fix the location of the warning to be on the use of i (so two different locations), instead of being on the whole external declaration? (cc @Armael, who is interested in multi-location compiler messages.)

This comment has been minimized.

Copy link
@Armael

Armael Jul 27, 2018

Member

I'm not sure we are going to get error messages with one message for several locations soon. In the situation here, it seems to me a simpler alternative would be to avoid emitting duplicated warnings -- and in that case highlighting the whole definition is good enough in a first approach. (the warning would still be emitted several times if there are several different types for which it applies)

This comment has been minimized.

Copy link
@gasche

gasche Jul 28, 2018

Member

My idea was to have two warnings shown, one with both the left i's location and the whole external location, and the other with the right i's location and the whole external location.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

Thanks! You probably also need to remove the test.*.reference files -- this will make it easier to eyeball the diff as currently only the expect-tests output appear there.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

I apologize for making you do so much fixup work, but I don't think that ocaml-typo=very-long-line is the right long-term change for test.ml. There seems to be a problem with some error/warning messages not being properly wrapped to the requested terminal width, probably due to missing format boxes. The problematic messages are the following:

Error: The native code version of the primitive is mandatory when attributes [@untagged] or [@unboxed] are present
Error: Don't know how to unbox this type. Only float, int32, int64 and nativeint can be unboxed
Error: The attribute '@unboxed' should be attached to a direct argument or result of the primitive, it should not occur deeply into its type

Would you be patient enough to find where those messages are emitted in the codebase, and add a text-wrap behavior? This is probably as simple as imitating the format boxes in other warnings/errors reports of the same file.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

This is getting slightly out of hands @gasche: any one even remotely familiar with the state of the compiler knows that this could go on forever, there is always "that other thing that doesn't really concern you but is so close to what you're doing that you should fix it as well while you're at it".

I personally think asking someone to learn how to use format to fix an error message they didn't write in the first place is definitely going too far.

@Armael

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

I can do a PR that fixes the error messages after this one gets merged, if you want.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

@trefis: I'm not much more comfortable with the dynamics of "let's ask @Armael to do this on his free time during his holidays instead of @smuenzel-js".

The compiler is like a shared house, we can all lend a hand in cleaning up some of the untidy parts when we meet them. It's always ok to say "no, I prefer to not do X right now", but pointing the issues out helps improve general quality and can encourage people to learn aspects of the codebase that they were less familiar with, which can be useful for their next time.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

The difference is that no one asked Armael anything, he volunteered.
Just like smuenzel volunteered to dust off your virtual carpet; don't act like he just came to grab a virtual beer in your virtual fridge.

It's always ok to say "no, I prefer to not do X right now", but pointing the issues out helps improve general quality and can encourage people to learn aspects of the codebase that they were less familiar with, which can be useful for their next time.

Sure. But keep in mind that you clearly have the upper hand in that discussion. They could object to your requests, but they don't know if by doing that they're just giving up any hope of seeing their work integrated. And you've been holding onto that power since the start of the discussion. You could have merged the initial contribution and then pointed out that "oh, it would also be nice to do this and that thing", but you haven't.

Anyway, let's stop blowing this out of proportions. It seems to me that you've made a bunch of requests, which I think were addressed, and I was just pointing out that I considered the last one unreasonable.

I'll let you resume reviewing this PR as you see fit.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

What would be useful for me is to hear from @smuenzel-js what he thinks is the right thing to do.

I'm not sure how to ask without "having the upper hand", but @trefis as you happen to know that I don't use my upper hands very often (and that I'm not the only one around willing to merge PRs), you could also help by encouraging your colleagues to come forward if they have opinions.

What I will do instead is fix the problem myself by force-pushing to this branch.

@smuenzel-js

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

We should prioritize fixing the bug in the description, and things that are directly related (e.g. producing more than one error for this bug).

The more changes we introduce that are unrelated to this fix, the more likely this PR will stall.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

I've pushed a change to your branch to use line-wrapping in the warnings/errors.

Thanks for 1d959e9; it may be useful to other people. It's a bit invasive for this patchset, but I reviewed it and believe it is correct -- in particular, thanks for being careful about perserving the evaluation order in the iter->fold change.

@gasche

gasche approved these changes Jul 30, 2018

Copy link
Member

left a comment

This is good to go if the CI passes.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

@smuenzel-js would you like to rebase the PR to cleanup the history, or could I do it myself? I think that it would be useful to keep the original fix, the one-error-per-typedecl change, and my format changes as separate commits. The testsuite changes can be squashed into a single commit, I believe.

@smuenzel-js

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

I tried to do this, but I don't think I'm familiar enough with git to pull it off properly, sorry.

@smuenzel-js smuenzel-js force-pushed the smuenzel-js:external-unboxed branch from 77d2071 to b075751 Jul 30, 2018

@smuenzel-js

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

trefis helped me do this

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

Thanks! Let's stop finicking and merge if the CI passes.

@gasche gasche merged commit 2c6d66c into ocaml:trunk Jul 31, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.