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 handling of warning attribute attached to type declarations #1977

Merged
merged 6 commits into from Aug 9, 2018

Conversation

Projects
None yet
2 participants
@nojb
Copy link
Contributor

commented Aug 9, 2018

Currently warning attributes attached to type declaration are not always respected. For example,

module M : sig end = struct type t = A [@@warning "-37"] end

will still trigger warning 37 if enabled.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

Will add a Changes entry once this is reviewed.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

The change looks obviously correct but the overall structure is a bit perplexing: among the many functions around this change that looks exactly the same, why is this one the only one protected in this way? It looks like either (1) the protection for other similar actions is done at a different level in the codebase, and your fix should go there as well or (2) this one is currently a special case because other store_* actions cannot raise a warning, but this property is fragile and maybe all functions should be protected.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Also, maybe there could be a regression test?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

It looks like either (1) the protection for other similar actions is done at a different level in the codebase, and your fix should go there as well or (2) this one is currently a special case because other store_* actions cannot raise a warning, but this property is fragile and maybe all functions should be protected.

I agree. My impression is that (2) is more like it. I am looking into it and will revise the patch afterwards.

@nojb nojb force-pushed the nojb:fix_warning_attributes_type_declarations branch from 49fef6f to dc5a38f Aug 9, 2018

nojb added some commits Aug 9, 2018

@nojb nojb force-pushed the nojb:fix_warning_attributes_type_declarations branch from ea57e1f to 14e4a87 Aug 9, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

OK, so it was more like (1) after all. I moved the fix to Typedecl, where warning scopes for the rest of the language categories are set.

The change assert falseignore in the second commit needs an explanation. The hash table Env.type_declarations maps the name of a type declaration to a function of type unit -> unit which is supposed to be called when when the type is used (this is what the function Env.mark_type_used does).

Entries in this hash table are inserted by the function Env.check_usage, and this only happens if the warning for which the usage information is wanted is activated. In the case of type declarations, Env.check_usage is called from Env.store_type.

The registered callback initially amounts to a boolean used flag but is modified to something more complicated to implement the unused check in the case of recursive definitions. This "modification" is done by updating the hash table using Env.set_type_used_callback. This function assumes an existing callback has already been registered for the type declaration (or, what amounts to the same thing, it assumes that it is called after check_usage and that the "unused type declaration" warning is activated).

However, this assumption does not hold anymore if we respect warning attributes attached to the individual type declarations. For if one of them disables the warning in question (Warning 34), then check_usage will not have entered the initial "used" callback and so the assertion in Env.set_type_used_callback will fail.

Instead of failing an assertion, we gracefully handle the case of a non-existing callback to account for this case.

Sounds reasonable?

More generally, it seems an approach where warnings are passed as arguments rather than handled indirectly via references would really simplify things. It will probably require a fair amount of refactoring, but I feel it would be worthwhile. I will look into it, but am interested if anyone has an opinion on this point.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

Link to #1248

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

Test added. Ready for review.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

The new patch still looks obviously correct to me.

I wondered if you couldn't change set_value_used_callback to have a more similar control flow to the new set_type_used_callback implementation (it was not possible before, but now they look similar), with of course the slight difference of the callback being of a different type. I don't insist that this should be done, however.
Also, set_value currently does not ignore declarations in ghost locations like set_type does, and I don't know whether it is a bug or an important feature, so they wouldn't be the same anyway. (If you're eager for more mysteries to solve...)

More generally, it seems an approach where warnings are passed as arguments rather than handled indirectly via references would really simplify things.

I certainly agree. I would note that this part of the codebase is really @alainfrisch's private garden. (But presumably he would still be happy if it was all cleaned up when he comes back from vacation.)

@gasche

gasche approved these changes Aug 9, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

(Do you know if the unused_types test has a good reason not to be an except-test?)

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

No idea.

@nojb nojb merged commit f95f8a2 into ocaml:trunk Aug 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nojb nojb deleted the nojb:fix_warning_attributes_type_declarations branch Aug 9, 2018

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.