Skip to content

Possible to use dproto in @safe code. #79

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.

Sign up for GitHub

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

Open
puzzlehawk wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Possible to use dproto in @safe code. #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions import/dproto/attributes.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import dproto.compat;
import std.traits : isArray, Identity;
import std.typecons : Nullable;

@safe:

struct ProtoField
{
string wireType;
Expand All @@ -40,6 +42,8 @@ template TagId(alias T)
template ProtoAccessors()
{

@safe:

Copy link
Contributor

@WebDrake WebDrake Jun 15, 2016

Choose a reason for hiding this comment

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

This shouldn't be applied to the templated methods; you can't guarantee it is appropriate.

I would suggest it only be applied to the non-templated serialize() method. Attribute inference will take care of the rest.

Copy link
Author

@puzzlehawk puzzlehawk Jun 17, 2016

Choose a reason for hiding this comment

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

OK. Didn't know about attribute inference :)
But still the templated deserialize() has to be marked as @safe too. Is it appropriate to require that the ProtoInputRange R provides @safe functions? My answer is yes, you should definitively enforce bound checks when parsing data from non-trusted sources.

static auto fromProto(R)(auto ref R data)
if(isProtoInputRange!R)
{
Expand Down
13 changes: 10 additions & 3 deletions import/dproto/serialize.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import std.range;
import std.system : Endian;
import std.traits;

@safe:

/*******************************************************************************
* Returns whether the given string is a protocol buffer primitive
*
Expand Down Expand Up @@ -254,6 +256,8 @@ long readVarint(R)(ref R src)
* r = output range
* src = The value to encode
* Returns: The created VarInt
*
* Note: This is @trusted for compatibility with @system R.put().
*/
void toVarint(R, T)(ref R r, T src) @trusted @property
if(isOutputRange!(R, ubyte) && isIntegral!T && isUnsigned!T)
Expand All @@ -279,6 +283,7 @@ void toVarint(R, T)(ref R r, T src) @trusted @property
* r = output range
* src = The value to encode
* Returns: The created VarInt
*
*/
void toVarint(R)(ref R r, long src) @safe @property
if(isOutputRange!(R, ubyte))
Expand Down Expand Up @@ -312,6 +317,8 @@ unittest {
* Params:
* src = The data stream
* Returns: The decoded value
*
* Note: This is @trusted for compatibility with @system R.put().
Copy link
Contributor

@WebDrake WebDrake Jun 15, 2016

Choose a reason for hiding this comment

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

Seems potentially problematic to me to blindly trust the code for any arbitrary R.

*/
T fromVarint(T = ulong, R)(R src) @property
if(isInputRange!R && is(ElementType!R : const ubyte) &&
Expand Down Expand Up @@ -460,19 +467,19 @@ void writeProto(string T, R)(ref R r, BuffType!T src)
}

/// Ditto
void writeProto(string T, R)(ref R r, const BuffType!T src)
void writeProto(string T, R)(ref R r, const BuffType!T src) @trusted
if(isProtoOutputRange!R &&
(T.msgType == "double".msgType || T.msgType == "float".msgType))
{
r.put(src.nativeToLittleEndian!(BuffType!T)[]);
}

/// Ditto
void writeProto(string T, R)(ref R r, const BuffType!T src)
void writeProto(string T, R)(ref R r, const BuffType!T src) @trusted
Copy link
Contributor

@WebDrake WebDrake Jun 15, 2016

Choose a reason for hiding this comment

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

I don't understand the rationale for making these @trusted. It in any case seems to me unwise in the case of a templated method.

Copy link
Author

@puzzlehawk puzzlehawk Jun 17, 2016

Choose a reason for hiding this comment

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

I don't feel that comfortable with @trusted here either. Better would be @safe which would require R.put() to be either safe or trusted. So it would be the users choice.
This could break compatibility to older code.

if(isProtoOutputRange!R && T.msgType == "string".msgType)
{
toVarint(r, src.length);
r.put(cast(ubyte[])src);
r.put(cast(const ubyte[]) src);
Copy link
Contributor

@WebDrake WebDrake Jun 15, 2016

Choose a reason for hiding this comment

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

What's the rationale for making this const ubyte[] instead of just regular ubyte[]?

Copy link
Author

@puzzlehawk puzzlehawk Jun 17, 2016

Choose a reason for hiding this comment

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

We don't expect put to make changes to src. Do we? Casting something const to something non-const feels itchy and is prohibited in @safe code.

}

/*******************************************************************************
Expand Down