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 issue 22038 - final switch error message should report all missing enum members #12709

Merged
merged 1 commit into from Jul 7, 2021

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Jun 18, 2021

I added errorSupplemental to Statement so each missing member can be printed on its own line, for easy copy-pasting into your source code (assuming your text-editor does multi-line editing). I also merged the previous final switch test-cases into one appropriately named one to make it more organized. If that's not desirable, I can revert it, just trying out things.

missingMembers = true;
ss.error("missing cases for `enum` members in `final switch`:");
}
ss.errorSupplemental("`%s`", em.toChars());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enum TOK has 235 members.

If I were to do:

final switch (tok)
{
    case TOK.null_:
        break;
}

The resultant compiler error would not be desirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I limit them to e.g. 10?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default is 6, or unlimited if -v(?) is passed. Take a look at the code that prints possible intended template matches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh there it is

const(uint) max_shown = !global.params.verbose ? 6 : uint.max;
copy that logic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done that now, but I don't know how to write a test for the verbose case since it puts absolute paths into the output, e.g.:

import    object	(/media/data1000/DebianHome/repos/dmd/test/../../druntime/import/object.d)
import    core.attribute	(/media/data1000/DebianHome/repos/dmd/test/../../druntime/import/core/attribute.d)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$p should help, see the README:

$p:<tail>$ paths ending with <tail> (which must refer to an existing file or directory)

EDIT: You could also filter those lines using TRANSFORM_OUTPUT: ... s.t. it also works for LDC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TRANSFORM_OUTPUT is better since I don't want the test to assert a specific order of semantic analysis. I'll steal the regex from fail15616b.d:

TRANSFORM_OUTPUT: remove_lines("^(predefs|binary|version|config|DFLAG|parse|import|semantic|entry|\s*$)")

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
22038 enhancement final switch error message should report all missing enum members

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12709"

@dkorpel dkorpel force-pushed the final-switch-err branch 4 times, most recently from 628514c to 4ef8dd1 Compare June 19, 2021 10:49
if (missingMembers > 0)
{
if (missingMembers > maxShown)
ss.errorSupplemental("... (%d more, -v to show) ...", missingMembers - maxShown);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ss.errorSupplemental("... (%d more, -v to show) ...", missingMembers - maxShown);
errorSupplemental(ss.loc, "... (%d more, -v to show) ...", missingMembers - maxShown);

There's nothing really gained from adding a new method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do Statement.error and Expression.errorSupplemental exist then? My guess was it is structured this way to make it easier to replace with something pure later on or something, but I don't know what the idea behind error member functions is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current error reporting system is archaic and should really be reworked to be cleaner and more flexible.

However, this is a different discussion. I suggest focusing on the minimal required changes for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed Statement.errorSupplemental now

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jul 6, 2021

@dkorpel circleCI has been hanging for a day. maybe re-push to restart it.

@ibuclaw
Copy link
Member

ibuclaw commented Jul 7, 2021

(Forcing restart of Azure pipelines).

@dlang-bot dlang-bot merged commit 51c52aa into dlang:master Jul 7, 2021
@dkorpel dkorpel deleted the final-switch-err branch August 31, 2021 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants