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

vibe-0.8.*: @safe delegate in Bson's opApply (foreach iterating) #1688

Closed
denizzzka opened this Issue Feb 21, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@denizzzka
Contributor

denizzzka commented Feb 21, 2017

Can not compile code what uses foreach over Bson array elements:

foreach(bElem; bsonArr)
{...}
../.dub/packages/dpq2-0.6.20/dpq2/src/dpq2/conv/from_bson.d(127,9): Error: none of the overloads of 'opApply' are callable using argument types (int delegate(Bson bElem) @system), candidates are:
../vibe.d/data/vibe/data/bson.d(662,6):        vibe.data.bson.Bson.opApply(scope int delegate(Bson obj) @safe del)
../vibe.d/data/vibe/data/bson.d(678,6):        vibe.data.bson.Bson.opApply(scope int delegate(ulong idx, Bson obj) @safe del)
../vibe.d/data/vibe/data/bson.d(700,6):        vibe.data.bson.Bson.opApply(scope int delegate(string idx, Bson obj) @safe del)

Also, identic(?) code for Json isn't contains @safe modifiers:
https://github.com/rejectedsoftware/vibe.d/blob/7f5175a21bceac49e4c52f62368c7ccecd895be6/data/vibe/data/json.d#L393

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 21, 2017

Member

This is really an issue. I found no way to have an opApply that works with type inference, as well as @safe inference - leaving off @safe requires littering @safe code with @trusted when using Bson, and adding it requires the same for @system code inside loop bodies.

Member

s-ludwig commented Feb 21, 2017

This is really an issue. I found no way to have an opApply that works with type inference, as well as @safe inference - leaving off @safe requires littering @safe code with @trusted when using Bson, and adding it requires the same for @system code inside loop bodies.

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Feb 21, 2017

Contributor

Probably, Dlang hasn't contains way to specify safe foreach delegate?
(I am tried to add @safe: for entry module with foreach statement, but this compilation error isn't gone.)

Contributor

denizzzka commented Feb 21, 2017

Probably, Dlang hasn't contains way to specify safe foreach delegate?
(I am tried to add @safe: for entry module with foreach statement, but this compilation error isn't gone.)

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Feb 21, 2017

Contributor

Probably, Dlang hasn't contains way to specify safe foreach delegate?

same case: http://forum.dlang.org/post/n8ggs2$1d67$1@digitalmars.com

Contributor

denizzzka commented Feb 21, 2017

Probably, Dlang hasn't contains way to specify safe foreach delegate?

same case: http://forum.dlang.org/post/n8ggs2$1d67$1@digitalmars.com

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Feb 21, 2017

Contributor

Bugzilla already contains bug description: https://issues.dlang.org/show_bug.cgi?id=15624

We need some workaround. Maybe, will help to create range struct for working with Bson?

Contributor

denizzzka commented Feb 21, 2017

Bugzilla already contains bug description: https://issues.dlang.org/show_bug.cgi?id=15624

We need some workaround. Maybe, will help to create range struct for working with Bson?

s-ludwig added a commit that referenced this issue Feb 26, 2017

Use ranges for DictionaryList foreach iteration.
This hopefully provides a better trade-off w.r.t. safe annotations and code breakage. Code that iterates over values only, needs to be adjusted to call .byValue, but all other code will infer safety properly.

See also #1688

s-ludwig added a commit that referenced this issue Feb 27, 2017

Add Bson.by[Key/Index]Value properties. See #1688.
The new properties perform proper safety inference. The old opApply() overloads have been reverted to take system delegates, to avoid mayor breakage of old code.
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 27, 2017

Member

I've now reverted opApply to take a @system delegate (at to be @system itself) - the breakage was too severe. Since I couldn't find a solution that would make type and attribute inference work, especially if multiple possibilities loop variables are involved, I've resorted to defining byValue, byKeyValue and byIndexValue ranges that are @safe.

Member

s-ludwig commented Feb 27, 2017

I've now reverted opApply to take a @system delegate (at to be @system itself) - the breakage was too severe. Since I couldn't find a solution that would make type and attribute inference work, especially if multiple possibilities loop variables are involved, I've resorted to defining byValue, byKeyValue and byIndexValue ranges that are @safe.

s-ludwig added a commit that referenced this issue Feb 27, 2017

s-ludwig added a commit that referenced this issue Feb 27, 2017

Add (J/B)son.by[Key/Index]Value properties.
This enables writing forward compatible code for 0.7.x and 0.8.x. See #1688.

s-ludwig added a commit that referenced this issue Feb 27, 2017

Remove trusted annotation from Json.opApply. See #1688.
This removes the safety hole that was introduced earlier.
@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Feb 27, 2017

Contributor

Maybe remove opApply at all? Since it @system and have ranges analog.

Contributor

denizzzka commented Feb 27, 2017

Maybe remove opApply at all? Since it @system and have ranges analog.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 2, 2017

Member

That should probably be done indeed, but later on, with a full deprecation phase.

Member

s-ludwig commented Mar 2, 2017

That should probably be done indeed, but later on, with a full deprecation phase.

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Mar 2, 2017

Contributor

I think, range completely substitutes opApply and users will not notice anything. It is wrong?

Contributor

denizzzka commented Mar 2, 2017

I think, range completely substitutes opApply and users will not notice anything. It is wrong?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 3, 2017

Member

They'll have to change from foreach (string k, v; bson) to foreach (k, v,; bson.byKeyValue). Unfortunately it is not possible to have multiple range types in one composite and still have foreach inference work.

Member

s-ludwig commented Mar 3, 2017

They'll have to change from foreach (string k, v; bson) to foreach (k, v,; bson.byKeyValue). Unfortunately it is not possible to have multiple range types in one composite and still have foreach inference work.

@s-ludwig s-ludwig closed this Mar 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment