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

[REG2.051] Issue 12420 - [AA] Can't set associative array with array as key value using key type #3408

Closed
wants to merge 1 commit into from

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Mar 30, 2014

Type *t2b = e2->type->toBasetype();
if (t2b->ty == Tarray && t2b->nextOf()->isMutable())
if (t2b->ty == Tarray && !e2->implicitConvTo(taa->index->immutableOf()))
Copy link
Member

Choose a reason for hiding this comment

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

This looks more correct.

@yebblies
Copy link
Member

I expect we're going to see a lot more like the phobos breakage in real-world code. I'm not sure requiring AA keys to be immutable was a good idea.

Also, could you track down that stray function std.net.curl._basicHTTP!char._basicHTTP no return exp; or assert(0); at end of function in the compiler output? I always see that when working on DDMD but it's too hard to isolate in that codebase.

@denis-sh
Copy link
Contributor

Please, no. See my Issue 12420 Comment 3. Let's just fix Issue 2954 which will auto-fix Issue 12420 and leave enhancement 12491 for future discussion. Considering code-breakage it will cause I don't expect it will be accepted. Looks like it's too late for D associative arrays to become correct.

@yebblies
Copy link
Member

Hey, you ask, you get. Don't get cold feet now it's happening.

The 'immutable' in the error message didn't match the check, but it certainly matched the intent.

@denis-sh
Copy link
Contributor

Hey, you ask, you get. Don't get cold feet now it's happening.

Sorry for being unclear. But now enhancement 12491 is filed and open for discussion.

The 'immutable' in the error message didn't match the check, but it certainly matched the intent.

I don't like reading one's mind without his will.

@denis-sh
Copy link
Contributor

Hey, you ask, you get. Don't get cold feet now it's happening.

No, I finally con't agree at all with this statement. I proposed to "remove the hack" in the original issue. I can't see how it can be treated as an indention to leave and even make the hack more strong as it's just totally inconsistent to select arrays from all types with indirections and check only it for immutability.

@yebblies
Copy link
Member

This is removing the hack, and partially implementing the enhancement.

@denis-sh
Copy link
Contributor

This is removing the hack

For me "hack" is inconsistent selection of arrays from all types with indirections.

…value using key type

And add test case for issue 2954.
@9rnsr
Copy link
Contributor Author

9rnsr commented Jun 21, 2014

OK, I misunderstood the described issue in 12420. In D, all AA keys must not be mutable. Therefore, accepting char[] as AA key type is invalid.

@9rnsr 9rnsr closed this Jun 21, 2014
@9rnsr
Copy link
Contributor Author

9rnsr commented Jun 21, 2014

Note that, this change itself is still valid independently from the issue 12420. But it will break some existing code. So I close this PR.

ibuclaw pushed a commit to ibuclaw/dmd that referenced this pull request Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants