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
Fix Issue 18809 - Improve error message on nonexistent property #10604
Conversation
Thanks for your pull request and interest in making D better, @alexandrumc! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#10604" |
src/dmd/typesem.d
Outdated
@@ -2158,8 +2159,32 @@ Expression getProperty(Type t, const ref Loc loc, Identifier ident, int flag) | |||
{ | |||
if (const n = importHint(ident.toString())) | |||
error(loc, "no property `%s` for type `%s`, perhaps `import %.*s;` is needed?", ident.toChars(), mt.toChars(), cast(int)n.length, n.ptr); | |||
else | |||
error(loc, "no property `%s` for type `%s`", ident.toChars(), mt.toChars()); | |||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else
{
src/dmd/typesem.d
Outdated
error(loc, "no property `%s` for type `%s`", ident.toChars(), mt.toChars()); | ||
else { | ||
s = mt.toDsymbol(null); | ||
if (s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this whole block can be factored into a function:
error(loc, "no property `%s` for type `%s%s`", ident.toChars(), tryGetModuleQualifier(mt), mt.toChars()))
// somewhere else
string tryGetModuleQualfier(Type mt)
{
// try to create and return a string to qualify mt
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to special case. Either we output the fully qualified name at all times or we don't.
In this situation it is better to output the fully qualified name even if it results in verbose outputs in the case of template instances. Fully qualified names offer the full information about where the type is declared and also, in the case of template instances, the instantiation types.
88a18bc
to
99f138e
Compare
99f138e
to
8efe6e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minus the mentioned nit
8efe6e4
to
e7838f4
Compare
…g fully qualified name
e7838f4
to
ba748f7
Compare
@@ -1,6 +1,6 @@ | |||
/* TEST_OUTPUT: | |||
--- | |||
fail_compilation/fail17969.d(9): Error: no property `sum` for type `MapResult2!((b) => b)` | |||
fail_compilation/fail17969.d(9): Error: no property `sum` for type `fail17969.__lambda6!(int[]).__lambda6.MapResult2!((b) => b)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should special case Voldemort types in error messages after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, the type of errors that one typically gets when missing std.algorithm / std.range imports are surprisingly underrepresented in the dmd test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should. At least with this change you'd know where to look to find the specific Voldermort type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do that, in this particular situation, if you instantiate with another type (other than int
) you would have the same error message at the same line, giving you no hint at all. With this patch you will have additional information(with what types the template has been instantiated). Ideally, the instantiation location should also be outputted, but I think that this should be the topic of a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I didn't check the test case in question. I assumed that it follows a typical phobos Voldemort pattern, but there are no Voldemort types involved at all. Disregard my comment.
The current ”no property” error message does not provide information about the module where the type in question is defined, so I added this information.
The module in which a symbol is defined will be printed only for the symbols which are defined in a file and misused somewhere else. This means that for this example
the compiler will yield "Error: no property
nonexistent
for typeSomeStruct
"while for this example
the compiler will yield "Error: no property
foo
for typecore.memory.GC
"Templates were not included in this solution since their fully qualified names may get very complicated. Moreover, I observed that sometimes the basic names of the symbols are already fully qualified (for reference:
dmd/test/fail_compilation/test15897.d
Line 5 in 57cd63e
dmd/test/fail_compilation/test12228.d
Line 6 in 57cd63e