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

Issue 12652 - Non-constant hash initializers should have a special-case diagnostic #11598

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

adelavais
Copy link
Contributor

Hello! I am unsure if this is the best error message, but from what I can see the problem is not only for global [0] declaration (like in the issue), I discovered it in a struct [1], and it extends to classes [2] too. There is no error in main or in a function called by main [3].
[0]: https://run.dlang.io/is/RRJOBT
[1]: https://run.dlang.io/is/E53JkC
[2]: https://run.dlang.io/is/2jOMg6
[3]: https://run.dlang.io/is/Q2I4mN

@dlang-bot
Copy link
Contributor

dlang-bot commented Aug 20, 2020

Thanks for your pull request and interest in making D better, @adelavais! 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
12652 enhancement Non-constant hash initializers should have a special-case diagnostic

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#11598"

src/dmd/todt.d Outdated
*/
void visitAssocArrayLiteral(AssocArrayLiteralExp e)
{
e.error("Associative array literals currently cannot be initialized globally, in structs or in classes; try using 'enum' or a module constructor instead");
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is rather long, the sugession should be a second line using errorSuplemental.

Copy link
Contributor

Choose a reason for hiding this comment

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

The error message should rather mention that it's impossible to initialize AA at compile time instead of runtime (the difference between [1],[2] and [3],[4]).

(I assume that this distinction should be understandable for non-compiler developers but other suggestions are welcome]

Comment on lines 8 to 10
import std.range;
import std.stdio;
import std.traits;
Copy link
Contributor

Choose a reason for hiding this comment

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

No phobos in the test suite (it's also unused?)

string[A] t = [A.x : "aaa", A.y : "bbb"];
}

void main ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary?


struct S
{
string[A] t = [A.x : "aaa", A.y : "bbb"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well add another global AA at module scope.

@Geod24
Copy link
Member

Geod24 commented Aug 20, 2020

A few suggestion:

  • Use errorSupplemental: If you search for it in the source code, you'll see other usages. Usually we use error to print the error message, and errorSupplemental to suggest a resolution. One exception is printing instantiation stack trace.
  • enum is not going to cut it, because it's still compile time;
  • Try to make the message relevant to the code. In your example code, it's in a struct, so it should move to the struct constructor. Module constructor should only be suggested if the parent is a global scope. Check toParent for this.

@adelavais
Copy link
Contributor Author

Thank you both for your feedback! I will update the PR as soon as possible.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Nov 4, 2020

@adelavais are you still pursuing this?

@adelavais
Copy link
Contributor Author

@adelavais are you still pursuing this?

Yes, I am. I will try to send a PR over the weekend.

src/dmd/todt.d Outdated
*/
void visitAssocArrayLiteral(AssocArrayLiteralExp e)
{
e.error("Associative array literals currently cannot be initialized globally, in structs or in classes; try using 'enum' or a module constructor instead");
Copy link
Member

Choose a reason for hiding this comment

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

Error messages are not capitalized

Copy link
Member

Choose a reason for hiding this comment

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

The error message does not quite make sense. The one from the bug report is better:

Associative array literals currently cannot be used to initialize globals. Try using a module constructor instead.

Copy link
Member

Choose a reason for hiding this comment

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

And remove "currently".

Copy link
Member

Choose a reason for hiding this comment

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

They aren't actually globals, either. They are statically allocated.

@RazvanN7
Copy link
Contributor

@MoonlightSentinel @WalterBright @Geod24 I rebased this and addressed the concerns. Is this ready to go?

@RazvanN7
Copy link
Contributor

cc @dkorpel

@dlang-bot dlang-bot merged commit 817610b into dlang:master Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants