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.

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

Make safe error messages consistent #20654

Merged
merged 1 commit into from
Jan 8, 2025
+394 −377
Merged

Make safe error messages consistent #20654

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
2 changes: 1 addition & 1 deletion compiler/src/dmd/dcast.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3261,7 +3261,7 @@ Expression scaleFactor(BinExp be, Scope* sc)
if (eoff.op == EXP.int64 && eoff.toInteger() == 0)
{
}
else if (sc.setUnsafe(false, be.loc, "pointer arithmetic not allowed in @safe functions"))
else if (sc.setUnsafe(false, be.loc, "pointer arithmetic"))
{
return ErrorExp.get();
}
Expand Down
14 changes: 7 additions & 7 deletions compiler/src/dmd/dsymbolsem.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,7 @@ private extern(C++) final class DsymbolSemanticVisitor : Visitor
// Calculate type size + safety checks
if (dsym.storage_class & STC.gshared && !dsym.isMember())
{
sc.setUnsafe(false, dsym.loc, "__gshared not allowed in safe functions; use shared");
sc.setUnsafe(false, dsym.loc, "using `__gshared` instead of `shared`");
}

Dsymbol parent = dsym.toParent();
Expand Down Expand Up @@ -1146,22 +1146,22 @@ private extern(C++) final class DsymbolSemanticVisitor : Visitor

if (dsym.type.hasPointers()) // also computes type size
sc.setUnsafe(false, dsym.loc,
"`void` initializers for pointers not allowed in safe functions");
"`void` initializing a pointer");
else if (dsym.type.hasInvariant())
sc.setUnsafe(false, dsym.loc,
"`void` initializers for structs with invariants are not allowed in safe functions");
"`void` initializing a struct with an invariant");
else if (dsym.type.toBasetype().ty == Tbool)
sc.setUnsafePreview(global.params.systemVariables, false, dsym.loc,
"a `bool` must be 0 or 1, so void intializing it is not allowed in safe functions");
"void intializing a bool (which must always be 0 or 1)");
else if (dsym.type.hasUnsafeBitpatterns())
sc.setUnsafePreview(global.params.systemVariables, false, dsym.loc,
"`void` initializers for types with unsafe bit patterns are not allowed in safe functions");
"`void` initializing a type with unsafe bit patterns");
}
else if (!dsym._init &&
!(dsym.storage_class & (STC.static_ | STC.extern_ | STC.gshared | STC.manifest | STC.field | STC.parameter)) &&
dsym.type.hasVoidInitPointers())
{
sc.setUnsafe(false, dsym.loc, "`void` initializers for pointers not allowed in safe functions");
sc.setUnsafe(false, dsym.loc, "`void` initializers for pointers");
}
}

Expand Down Expand Up @@ -1323,7 +1323,7 @@ private extern(C++) final class DsymbolSemanticVisitor : Visitor
{
import dmd.escape : setUnsafeDIP1000;
const inSafeFunc = sc.func && sc.func.isSafeBypassingInference(); // isSafeBypassingInference may call setUnsafe().
if (setUnsafeDIP1000(*sc, false, dsym.loc, "`scope` allocation of `%s` requires that constructor be annotated with `scope`", dsym))
if (setUnsafeDIP1000(*sc, false, dsym.loc, "`scope` allocation of `%s` with a non-`scope` constructor", dsym))
errorSupplemental(ne.member.loc, "is the location of the constructor");
}
ne.onstack = 1;
Expand Down
66 changes: 34 additions & 32 deletions compiler/src/dmd/escape.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,17 +354,17 @@
if (assertmsg)
{
result |= sc.setUnsafeDIP1000(gag, arg.loc,
desc ~ " `%s` assigned to non-scope parameter calling `assert()`", v);
"assigning" ~ desc ~ " `%s` to non-scope parameter calling `assert()`", v);
return;
}

bool isThis = fdc && fdc.needThis() && fdc.vthis == vPar; // implicit `this` parameter to member function

const(char)* msg =
(isThis) ? (desc ~ " `%s` calling non-scope member function `%s.%s()`") :
(fdc && parId) ? (desc ~ " `%s` assigned to non-scope parameter `%s` calling `%s`") :
(fdc && !parId) ? (desc ~ " `%s` assigned to non-scope anonymous parameter calling `%s`") :
(!fdc && parId) ? (desc ~ " `%s` assigned to non-scope parameter `%s`") :
(fdc && parId) ? ("assigning " ~ desc ~ " `%s` to non-scope parameter `%s` calling `%s`") :
(fdc && !parId) ? ("assigning " ~ desc ~ " `%s` to non-scope anonymous parameter calling `%s`") :
(!fdc && parId) ? ("assigning " ~ desc ~ " `%s` to non-scope parameter `%s`") :
(desc ~ " `%s` assigned to non-scope anonymous parameter");

if (isThis ?
Expand Down Expand Up @@ -440,8 +440,8 @@
if (parStc & STC.scope_)
return;
const(char)* msg = parId ?
"reference to stack allocated value returned by `%s` assigned to non-scope parameter `%s`" :
"reference to stack allocated value returned by `%s` assigned to non-scope anonymous parameter";
"assigning reference to stack allocated value returned by `%s` to non-scope parameter `%s`" :
"assigning reference to stack allocated value returned by `%s` to non-scope anonymous parameter";

Check warning on line 444 in compiler/src/dmd/escape.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/escape.d#L444 

Added line #L444 was not covered by tests

result |= sc.setUnsafeDIP1000(gag, ee.loc, msg, ee, parId);
}
Expand Down Expand Up @@ -726,16 +726,16 @@
{
case EnclosedBy.none: assert(0);
case EnclosedBy.returnScope:
msg = "scope variable `%s` assigned to return scope `%s`";
msg = "assigning scope variable `%s` to return scope `%s`";

Check warning on line 729 in compiler/src/dmd/escape.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/escape.d#L729 

Added line #L729 was not covered by tests
break;
case EnclosedBy.longerScope:
msg = "scope variable `%s` assigned to `%s` with longer lifetime";
msg = "assigning scope variable `%s` to `%s` with longer lifetime";
break;
case EnclosedBy.refVar:
msg = "scope variable `%s` assigned to `ref` variable `%s` with longer lifetime";
msg = "assigning scope variable `%s` to `ref` variable `%s` with longer lifetime";
break;
case EnclosedBy.global:
msg = "scope variable `%s` assigned to global variable `%s`";
msg = "assigning scope variable `%s` to global variable `%s`";

Check warning on line 738 in compiler/src/dmd/escape.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/escape.d#L738 

Added line #L738 was not covered by tests
break;
}

Expand All @@ -762,7 +762,7 @@
}
return;
}
result |= sc.setUnsafeDIP1000(gag, ae.loc, "scope variable `%s` assigned to non-scope `%s`", v, e1);
result |= sc.setUnsafeDIP1000(gag, ae.loc, "assigning scope variable `%s` to non-scope `%s`", v, e1);
}
else
{
Expand Down Expand Up @@ -794,7 +794,7 @@
else
{
result |= sc.setUnsafeDIP1000(gag, ae.loc,
"address of local variable `%s` assigned to return scope `%s`", v, va);
"assigning address of local variable `%s` to return scope `%s`", v, va);
}
}

Expand All @@ -809,7 +809,7 @@
// If va's lifetime encloses v's, then error
if (va && !(vaIsFirstRef && v.isReturn()) && va.enclosesLifetimeOf(v))
{
if (sc.setUnsafeDIP1000(gag, ae.loc, "address of variable `%s` assigned to `%s` with longer lifetime", v, va))
if (sc.setUnsafeDIP1000(gag, ae.loc, "assigning address of variable `%s` to `%s` with longer lifetime", v, va))
{
result = true;
return;
Expand All @@ -829,7 +829,7 @@
return;
}

result |= sc.setUnsafeDIP1000(gag, ae.loc, "reference to local variable `%s` assigned to non-scope `%s`", v, e1);
result |= sc.setUnsafeDIP1000(gag, ae.loc, "assigning reference to local variable `%s` to non-scope `%s`", v, e1);
}

void onFunc(FuncDeclaration func, bool called)
Expand Down Expand Up @@ -869,7 +869,7 @@
return;
}
result |= sc.setUnsafeDIP1000(gag, ae.loc,
"reference to local `%s` assigned to non-scope `%s` in @safe code", v, e1);
"assigning reference to local `%s` to non-scope `%s`", v, e1);
}
}

Expand All @@ -889,8 +889,8 @@
}

const(char)* msg = (ee.op == EXP.structLiteral) ?
"address of struct literal `%s` assigned to `%s` with longer lifetime" :
"address of expression temporary returned by `%s` assigned to `%s` with longer lifetime";
"assigning address of struct literal `%s` to `%s` with longer lifetime" :
"assigning address of expression temporary returned by `%s` to `%s` with longer lifetime";

Check warning on line 893 in compiler/src/dmd/escape.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/escape.d#L893 

Added line #L893 was not covered by tests

result |= sc.setUnsafeDIP1000(gag, ee.loc, msg, ee, e1);
}
Expand Down Expand Up @@ -930,7 +930,7 @@
// despite being `scope`
{
// https://issues.dlang.org/show_bug.cgi?id=17029
result |= sc.setUnsafeDIP1000(gag, e.loc, "scope variable `%s` may not be thrown", v);
result |= sc.setUnsafeDIP1000(gag, e.loc, "throwing scope variable `%s`", v);

Check warning on line 933 in compiler/src/dmd/escape.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/escape.d#L933 

Added line #L933 was not covered by tests
return;
}
else
Expand Down Expand Up @@ -989,7 +989,7 @@
!(p.parent == sc.func))
{
// https://issues.dlang.org/show_bug.cgi?id=20868
result |= sc.setUnsafeDIP1000(gag, e.loc, "scope variable `%s` may not be copied into allocated memory", v);
result |= sc.setUnsafeDIP1000(gag, e.loc, "copying scope variable `%s` into allocated memory", v);
return;
}
}
Expand All @@ -1009,9 +1009,9 @@
bool escapingRef(VarDeclaration v, FeatureState fs)
{
const(char)* msg = v.isParameter() ?
"copying `%s` into allocated memory escapes a reference to parameter `%s`" :
"copying `%s` into allocated memory escapes a reference to local variable `%s`";
return setUnsafePreview(&sc, fs, gag, e.loc, msg, e, v);
"escaping a reference to parameter `%s` by copying `%s` into allocated memory" :
"escaping a reference to local variable `%s by copying `%s` into allocated memory";
Comment on lines -1012 to +1013
Copy link
Member

@ibuclaw ibuclaw Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @dkorpel - this caused a diagnostic regression - there's a missing closing backtick in this format string.

Are you able to fix this? Alternatively can include it in the bundle of "broken DMD/C++ API" things I'm preparing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, give me a minute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#20677

return setUnsafePreview(&sc, fs, gag, e.loc, msg, v, e);
}

Dsymbol p = v.toParent2();
Expand Down Expand Up @@ -1064,14 +1064,14 @@
{
if (called)
result |= sc.setUnsafeDIP1000(gag, e.loc,
"nested function `%s` returns `scope` values and escapes them into allocated memory", fd);
"escaping a `scope` value returned from nested function `%s` into allocated memory", fd);
}

void onExp(Expression ee, bool retRefTransition)
{
if (log) printf("byexp %s\n", ee.toChars());
if (!gag)
sc.eSink.error(ee.loc, "storing reference to stack allocated value returned by `%s` into allocated memory causes it to escape",
sc.eSink.error(ee.loc, "escaping reference to stack allocated value returned by `%s` into allocated memory",

Check warning on line 1074 in compiler/src/dmd/escape.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/escape.d#L1074 

Added line #L1074 was not covered by tests
ee.toChars());
result = true;
}
Expand Down Expand Up @@ -1210,7 +1210,7 @@
else
{
// https://issues.dlang.org/show_bug.cgi?id=17029
result |= sc.setUnsafeDIP1000(gag, e.loc, "scope variable `%s` may not be returned", v);
result |= sc.setUnsafeDIP1000(gag, e.loc, "returning scope variable `%s`", v);
return;
}
}
Expand All @@ -1233,13 +1233,12 @@
// depending on the flag passed to the CLI for DIP25
void escapingRef(VarDeclaration v, FeatureState featureState)
{
const(char)* msg = v.isParameter() ?
"returning `%s` escapes a reference to parameter `%s`" :
"returning `%s` escapes a reference to local variable `%s`";

const(char)* safeMsg = v.isParameter() ?
"escaping a reference to parameter `%s` by returning `%s`" :
"escaping a reference to local variable `%s` by returning `%s` ";
if (v.isParameter() && v.isReference())
{
if (setUnsafePreview(&sc, featureState, gag, e.loc, msg, e, v) ||
if (setUnsafePreview(&sc, featureState, gag, e.loc, safeMsg, v, e) ||
sc.func.isSafeBypassingInference())
{
result = true;
Expand All @@ -1260,10 +1259,13 @@
{
if (retRefTransition)
{
result |= sc.setUnsafeDIP1000(gag, e.loc, msg, e, v);
result |= sc.setUnsafeDIP1000(gag, e.loc, safeMsg, v, e);
}
else
{
const(char)* msg = v.isParameter() ?
"returning `%s` escapes a reference to parameter `%s`" :
"returning `%s` escapes a reference to local variable `%s`";
if (!gag)
previewErrorFunc(sc.isDeprecated(), featureState)(e.loc, msg, e.toChars(), v.toChars());
result = true;
Expand Down Expand Up @@ -2228,7 +2230,7 @@

// take address of `scope` variable not allowed, requires transitive scope
return sc.setUnsafeDIP1000(gag, e.loc,
"cannot take address of `scope` variable `%s` since `scope` applies to first indirection only", v);
"taking address of `scope` variable `%s` with pointers", v);
}

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