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

Allow const parameters in REST generator #2552

Merged
merged 1 commit into from Aug 3, 2021

Conversation

omerfirmak
Copy link
Contributor

This also allows in parameters to be used with
--preview=in.

@Geod24
Copy link
Contributor

Geod24 commented Apr 20, 2021

Casting const away is not the right approach. Rather, mutability should not be required.
To do this, the value have to be constructed in one go, not piece-meal. That's for example why our deserializer does this:
https://github.com/bosagora/serialization/blob/8f1e4c124e7c909e7b65ffea39e996fc116ba181/source/agora/serialization/Serializer.d#L691-L699

The staticMap means we end up calling the constructor, and give it function calls as parameter.
Another "nice" trick is how we deal with static arrays: https://github.com/bosagora/serialization/blob/8f1e4c124e7c909e7b65ffea39e996fc116ba181/source/agora/serialization/Serializer.d#L609-L617

Nowhere in the serializer do we have away constness (we do have cast, but they never remove const). While casting away constness is sometimes okay, modifying the underlying data (which this does) never is.

@omerfirmak
Copy link
Contributor Author

Updated

web/vibe/web/rest.d Outdated Show resolved Hide resolved
@Geod24
Copy link
Contributor

Geod24 commented Apr 20, 2021

Test plz

@omerfirmak
Copy link
Contributor Author

Test plz

Added

@Geod24
Copy link
Contributor

Geod24 commented Jul 28, 2021

web-framework-i18n-example ~master: building configuration "application"...
Compiling Diet HTML template home.dt...
../../web/vibe/web/i18n.d(222,4): Error: `"Missing translation key for en_US; home: This is the internationalization example app."`
../../web/vibe/web/i18n.d(175,24):        called from here: `tr(key, null, 0, context)`
../../web/vibe/web/web.d(340,99):        called from here: `tr(key, context)`
/home/runner/.dub/packages/diet-ng-1.8.0/diet-ng/source/diet/traits.d(75,23):        called from here: `translate(text, context)`
/home/runner/.dub/packages/diet-ng-1.8.0/diet-ng/source/diet/parser.d(1239,17):        called from here: `translate(kln, stripExtension(baseName(loc.file)))`
/home/runner/.dub/packages/diet-ng-1.8.0/diet-ng/source/diet/parser.d(1110,20):        called from here: `parseTextLine(remainder, ret, tmploc)`
/home/runner/.dub/packages/diet-ng-1.8.0/diet-ng/source/diet/parser.d(1044,38):        called from here: `parseTagLine(input, loc, has_nested)`
/home/runner/.dub/packages/diet-ng-1.8.0/diet-ng/source/diet/parser.d(77,76):        called from here: `parseDietRaw(f)`

@s-ludwig : Looks like our CI is broken ?

@Geod24
Copy link
Contributor

Geod24 commented Jul 29, 2021

Indeed, it's also triggering in Buildkite: dlang/dmd#12526 (comment)

@PetarKirov
Copy link
Contributor

@Geod24 I think the cause may have been this PR: rejectedsoftware/diet-ng#92

@Geod24
Copy link
Contributor

Geod24 commented Aug 3, 2021

@omerfirmak : Please rebase

@omerfirmak
Copy link
Contributor Author

Rebased

@Geod24
Copy link
Contributor

Geod24 commented Aug 3, 2021

[INFO] Running test rest
+ cd tests/rest
+ dub --compiler=dmd --build-mode=separate
Running pre-generate commands for vibe-d:tls...
Performing "debug" build using dmd for x86_64.
[...]
tests ~master: building configuration "application"...
../../web/vibe/web/rest.d(1609,6): Error: static assert:  `sroute.parameters[0].kind == ParameterKind.header` is false
../../web/vibe/web/rest.d(442,45):        instantiated from here: `jsonMethodHandler!(constRefFoo, 1LU, Example8)`
source/app.d(721,23):        instantiated from here: `registerRestInterface!(Example8)`
dmd failed with exit code 1.

@omerfirmak
Copy link
Contributor Author

What, dlang/dmd@2333339 should've fixed it.

@Geod24
Copy link
Contributor

Geod24 commented Aug 3, 2021

dlang/dmd@03c3060

Only for v2.096.1 and above (LDC 1.26). So you need to version out the test for anything less than that (we test with 10 versions of DMD). But it also fails on dmd-latest so no luck here.

@@ -300,7 +301,7 @@ import std.traits : hasUDA;
pi.isOut = true;
} else static if (SC & ParameterStorageClass.ref_) {
pi.isIn = true;
pi.isOut = true;
pi.isOut = isMutable!PT;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's see if this will help. Works fine on LDC 1.27.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hold on, we use inIn for "input parameter", not the in storage class, right ? Damn that's confusing :D

@Geod24 Geod24 enabled auto-merge August 3, 2021 09:36
@Geod24 Geod24 merged commit 8620cbd into vibe-d:master Aug 3, 2021
@omerfirmak omerfirmak deleted the rest-in-param branch August 3, 2021 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants