Issue 15856 - Confusing error message with -transition=checkimports #5651
Conversation
|
|
Do we really need to change 19 files to change a message? Something seems wrong. |
|
@MartinNowak: Could you have a short look at this? |
Yes we must fix the unsync between C++ headers and D declarations that was introduced in #5445. It's definitely wrong, because when a Today, |
| fail_compilation/checkimports2c.d(30): Deprecation: local import search method found variable imports.imp2.Y instead of variable imports.imp1.Y | ||
| fail_compilation/checkimports2c.d(22): Deprecation: local import search method found variable imports.imp2.X instead of variable checkimports2c.X | ||
| fail_compilation/checkimports2c.d(28): Deprecation: local import search method found variable imports.imp2.X instead of nothing | ||
| fail_compilation/checkimports2c.d(29): Deprecation: local import search method found variable imports.imp2.Y instead of nothing |
There was a problem hiding this comment.
Looking at the test case, those don't seem very clear.
Could we have something that isn't refering to the compiler internal, e.g:
Deprecation: Variable imports.imp2.Y introduced by a private local import is not visible with the new import rule, use a public import or an alias ?
Other suggestions welcome, but "found X instead of nothing" is not user friendly.
There was a problem hiding this comment.
We don't know through which import we found the symbol, and it would be too much work to figure out.
This appears to be a separate issue from fixing a confusing error message. Hence, it should go in a separate PR. Please, please, have only one issue per PR. Do not lump multiple things together. I've posted the reasons at least a dozen times. I don't think there's any risk of overflowing the PR # counter :-) |
|
@WalterBright Done. #5657 |
|
When I've started to fix issue 15856, the default argument mismatch had blocked to change the default arguments to tweak search method in some places. Fortunately the final change didn't need that method, but it's still a serious bug, and related with the |
9rnsr
force-pushed
the
fix15856
branch
3 times, most recently
from
6f650ff
to
b77b092
Compare
* Add -transition=chekcimporrts logic in Type(Struct|Class).dotExp * Stop confusing deprecation message "... found in local import" message in ScopeDsymbol.search
|
|
||
| Dsymbol searchSym() | ||
| { | ||
| int flags = 0; |
There was a problem hiding this comment.
No need to declare flags here, just use 0 and SearchLocalsOnly directly.
|
This also seems to fix Issue 15897 – private base class method not seen through derived class like my PR #5727, true? |
| @@ -7868,14 +7868,38 @@ public: | |||
| // goto L1; | |||
| } | |||
| } | |||
| s = sym.search(e.loc, ident); | |||
There was a problem hiding this comment.
Yes, I think we should get rid of the default flags for Dsymbol.search. They even differ for Dsymbol and ScopeDSymbol :o.
It's changing 6 files, a few headers and tests. And it also adds the old vs. new import check to TypeClass and TypeStruct. Seems fairly reasonable to me. |
| else | ||
| { | ||
| flags |= SearchLocalsOnly; | ||
| } |
There was a problem hiding this comment.
Rather do
if (!(flags & (SearchLocalsOnly | SearchImportsOnly))
flags |= SearchLocalsOnly;
|
I've put my comments into a PR 9rnsr#6. |
remove dead comments
|
I merged 9rnsr#6, and then fixed a breaking introduced by it. |
Improve deprecation message mechanism for next minor release 2.071.1.