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

String enum as form parameter is treated as an array of `immutable(char)` #2100

Merged
merged 4 commits into from Mar 12, 2018

Conversation

Projects
None yet
2 participants
@CromFr
Copy link
Contributor

CromFr commented Mar 5, 2018

Using a string enum as parameter in a web interface results in build failure.

Example:

import vibe.d;

enum StrEnum{
	a = "hello", b = "world"
}
class WebIface{
	void getTest(StrEnum param){}
}

void main(){
	import std.traits;
	pragma(msg, "StrEnum is dynamic array: ", isDynamicArray!StrEnum);
	pragma(msg, "StrEnum is some string: ", isSomeString!StrEnum);

	auto router = new URLRouter;
	router.registerWebInterface(new WebIface);
}

Outputs:

StrEnum is dynamic array: true
StrEnum is some string: false
vibe-d-0.8.3-rc.1/vibe-d/web/vibe/web/common.d(799,9): Error: cannot modify `immutable` expression `dst`
vibe-d-0.8.3-rc.1/vibe-d/web/vibe/web/common.d(773,15): Error: template instance `vibe.web.common.setVoid!(immutable(char), immutable(char))` error instantiating
vibe-d-0.8.3-rc.1/vibe-d/web/vibe/web/common.d(737,23):        instantiated from here: `webConvTo!(immutable(char))`
vibe-d-0.8.3-rc.1/vibe-d/web/vibe/web/common.d(704,29):        instantiated from here: `readFormParamRec!(immutable(char))`
vibe-d-0.8.3-rc.1/vibe-d/web/vibe/web/web.d(927,40):        instantiated from here: `readFormParamRec!(StrEnum)`
vibe-d-0.8.3-rc.1/vibe-d/web/vibe/web/web.d(185,34):        instantiated from here: `handleRequest!("getTest", getTest, WebIface)`
source/app.d(16,29):        instantiated from here: `registerWebInterface!(WebIface, cast(MethodStyle)5)`

This is due to this condition being true when the variable is an enum with string as the underlying type.

This means the parameter is treated as an array of immutable(char), and cause later failure when it tries to set an immutable value (I don't know if it's intended, but immutable form parameters always produce this "set immutable" error)

This change fixed the issue for my use case, but it could have some naughty side effects that I haven't thought of.

@CromFr

This comment has been minimized.

Copy link
Contributor Author

CromFr commented Mar 11, 2018

This bug is caused by this commit on Phobos: dlang/phobos@b0df44c

With this in mind, using (isSomeString!T || (is(T == enum) && is(T: string))) (instead of what is in the PR) should be closer to the previous behavior

@s-ludwig

This comment has been minimized.

Copy link
Member

s-ludwig commented Mar 11, 2018

Thanks for investigating this further! Perhaps even better as a fix would be isSomeString!(OriginalType!T) (OriginalType returns the type underlying the enum). I'll prepare a PR shortly.

edit: I didn't notice that this was already a PR!

@CromFr

This comment has been minimized.

Copy link
Contributor Author

CromFr commented Mar 12, 2018

OriginalType makes the solution much more elegant :)

CromFr added some commits Mar 12, 2018

@CromFr

This comment has been minimized.

Copy link
Contributor Author

CromFr commented Mar 12, 2018

I shouldn't use the Github editor :)

@s-ludwig

This comment has been minimized.

Copy link
Member

s-ludwig commented Mar 12, 2018

Thanks, looks good to merge now (the CI result is currently very unreliable unfortunately).

@s-ludwig s-ludwig merged commit 235069f into vibe-d:master Mar 12, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
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.