Bug 199 – (D1 only) Label causes scope to collapse into parent

When using a label directly before an anonymous scope (set of curly brackets), it causes the anonymous scope to behave as though its contents are declared in the parent scope directly. This causes variable names to collide should they share the same name between child and parent: void main(){ uint foo; x: { uint foo; } } test.d(5): declaration test.main.foo is already defined Adding '{}' immediately after the label seems to be a viable workaround.

DMD 0.161 disallows shadowing of local vars. So this is now a non-issue.

The scope is still being collapsed. Added to DStress as http://dstress.kuehne.cn/run/s/scope_13_A.d http://dstress.kuehne.cn/run/s/scope_13_B.d

yeah, 1.0 doesn't allow shadowing of local var. it's not an issue anymore. and don't expect walter would fix the old revision

David, I appreciate your trying to help, but this bug is NOT fixed. The scope_13_A test case that Thomas added is still failing. You can see that either by trying it yourself (which I just did), or looking at the test results here: http://dstress.kuehne.cn/www/dmd-1.00.html

On the basis of what you say, the compiler is behaving according to the spec, which states http://www.digitalmars.com/d/statement.html#LabeledStatement LabelledStatement: Identifier ':' NoScopeStatement I'd noticed it before. But since this bit of the spec is presumably not what was intended, I'm leaving this issue open. If there's any temptation to hit the INVALID button, it should be changed to an enhancement request instead.

Since the compiler is behaving according to spec, I don't want to change this because it could break existing code, and there doesn't seem to be a compelling reason to do so.

this still fail: http://dstress.kuehne.cn/run/s/scope_13_A.d

*** Bug 217 has been marked as a duplicate of this bug. ***

The test case behaves as expected, because labels do not introduce a new scope when followed by { }.

According to this talk: http://forum.dlang.org/thread/[email protected]#post-axonofactkyzpagilcbm:40forum.dlang.org The spec does NOT state it should work that way. Even if it did: - Doing it this way buys nothing, as you get the same result simply by... not typing the block - Is bug prone (accidental labeling) - Potentially creates different behavior from C code. NB: The original example is wrong because of shadowing. But this one is more viable. //---- void main() { { uint foo; } end_first_block: { uint foo; } end_second_block: { uint foo; //main.d(12): Error: declaration main.main.foo is already defined } end_third_block: return; } //----

(In reply to comment #10) > - Potentially creates different behavior from C code. What are the conditions for it to create different behaviour? Maybe you can show two programs, in C and D, that behave differently.

(In reply to comment #11) > (In reply to comment #10) > > > - Potentially creates different behavior from C code. > > What are the conditions for it to create different behaviour? Maybe you can > show two programs, in C and D, that behave differently. This program for example, will behave differently in C and D: -------- -------- int i = 3; void main() { { int some_condition = 1; if ( some_condition ) goto block_end; /* dummy code */ } block_end: { int i = 7; printf("%i", i); //Local i } printf("%i", i); //Global i (?) } -------- C prints: "70" D prints: "77" -------- The conditions needed to create it is mostly just un-expected shadowing. This one shows difference with C, but even without C, the behavior is not what would have been expected from a D developer anyway. The issue can also be reproduced in member functions, that have local variables that shadow members. The shadowing will outlive its scope, and create unexpected behavior.

(In reply to comment #12) > int i = 3; > > void main() > { > { > int some_condition = 1; > if ( some_condition ) > goto block_end; > > /* dummy code */ > } block_end: > > { > int i = 7; > printf("%i", i); //Local i > } > > printf("%i", i); //Global i (?) > } > -------- > C prints: "70" > D prints: "77" > -------- With GCC this C code gives me "73": http://codepad.org/7uKsouJ0 http://ideone.com/GBiRCX //import core.stdc.stdio: printf; #include "stdio.h" int i = 3; int main() { { int some_condition = 1; if (some_condition) goto block_end; /* dummy code */ } block_end: { int i = 7; printf("%i", i); //Local i } printf("%i", i); // Global i (?) return 0; }

(In reply to comment #13) > With GCC this C code gives me "73": Yes, my bad. Copy paste typo.

comment #1: > Adding '{}' immediately after the label seems to be a viable workaround. Or moving the label inside the block: { label: /* code */ } Walter Bright, comment #6: > I don't want to change this because it could break existing code We could deprecate having a BlockStatement after a label, with a message suggesting to move the label within the block. That way no code gets broken.

(In reply to comment #15) > Walter Bright, comment #6: > > I don't want to change this because it could break existing code > > We could deprecate having a BlockStatement after a label, with a message > suggesting to move the label within the block. That way no code gets broken. Wait. What? Deprecate having a block statement after a label? Why would we do that? That's completely arbitrary. Plus, that would *definitely* break more code than what we'd break with a straight up fix. This behavior was never according to spec anyways. I say we just fix it.

(In reply to comment #10) > According to this talk: > http://forum.dlang.org/thread/[email protected]#post-axonofactkyzpagilcbm:40forum.dlang.org > The spec does NOT state it should work that way. Yes it does, as I quoted in comment 5. LabeledStatement: Identifier : NoScopeStatement But since - it's arbitrary - it's counter-intuitive - I can't see how any code can take advantage of this "feature" I agree in principle that it should be changed to simply LabeledStatement: Identifier : Statement While this would change the behaviour of code such as that in comment 13, ISTM far more likely that anybody who seriously writes such code is unfamiliar with this arcane detail of the D spec. (In reply to comment #16) > (In reply to comment #15) >> Walter Bright, comment #6: >>> I don't want to change this because it could break existing code >> >> We could deprecate having a BlockStatement after a label, with a message >> suggesting to move the label within the block. That way no code gets broken. > Wait. What? Deprecate having a block statement after a label? Why would we do > that? That's completely arbitrary. Plus, that would *definitely* break more > code than what we'd break with a straight up fix. My inkling is that, compared with the amount of code it would break, it would cause far more code that was already broken to generate a compiler error.

(In reply to comment #17) > (In reply to comment #10) > > According to this talk: > > http://forum.dlang.org/thread/[email protected]#post-axonofactkyzpagilcbm:40forum.dlang.org > > The spec does NOT state it should work that way. > > Yes it does, as I quoted in comment 5. > > LabeledStatement: > Identifier : NoScopeStatement That was my statement :D but it was rebuked by Ali in comment 7 that: It is still a bug because NoScopeStatement does not mean "expand into current scope." It means "do not introduce a scope" and clearly allows blocked statements: NoScopeStatement: ; NonEmptyStatement BlockStatement > (In reply to comment #16) > > (In reply to comment #15) > >> Walter Bright, comment #6: > >>> I don't want to change this because it could break existing code > >> > >> We could deprecate having a BlockStatement after a label, with a message > >> suggesting to move the label within the block. That way no code gets broken. > > Wait. What? Deprecate having a block statement after a label? Why would we do > > that? That's completely arbitrary. Plus, that would *definitely* break more > > code than what we'd break with a straight up fix. > > My inkling is that, compared with the amount of code it would break, it would > cause far more code that was already broken to generate a compiler error. Isn't "code that was broken now generates a compiler error" good though...? I still maintain that the amount of use case in nature is probably trivially low. Of those, the amount it would break even lower. I have no proof, but D is modern, and modern tends to stay clear away of goto and labels anyways.

(In reply to comment #18) > It is still a bug because NoScopeStatement does not mean "expand into > current scope." It means "do not introduce a scope" What is the distinction you are making here? > Isn't "code that was broken now generates a compiler error" good though...? It is, and that's part of the point I was trying to make.

(In reply to comment #19) > (In reply to comment #18) > > It is still a bug because NoScopeStatement does not mean "expand into > > current scope." It means "do not introduce a scope" > > What is the distinction you are making here? Well, if it was a "ScopedStatement", it would have meant that: ----- label: int i; //i is scoped here i = 5; //Error, i has not been declared ----- In contrast, with a NoScopedStatement, all it means is that when you write: ----- label: "{stuff}" ----- It means that that "{stuff}" itself is not scoped: the "{stuff}" statement can be seen from the outside. But that doesn't mean the "{stuff}" itself doesn't create its own internal scope, which contains "stuff", and which can't be seen from the outside... That's how I read it now, and the explanation Ali was making. > > Isn't "code that was broken now generates a compiler error" good though...? > > It is, and that's part of the point I was trying to make. Ok, cool. Since it was a reply to me, it seemed you were arguing against it.

The only statement nodes that create a scope according to the spec are ScopeStatement and ScopeBlockStatement. When you have Identifier : { StatementList } the structure is LabeledStatement Identifier : NoScopeStatement BlockStatement { StatementList } No node that creates a scope here. It's the same way with the conditional compilation statements. Of course, statements within the StatementList may introduce their own scopes, but nothing in this parse tree as it stands creates a scope.

(In reply to comment #21) > The only statement nodes that create a scope according to the spec are > ScopeStatement and ScopeBlockStatement. When you have > > Identifier : { StatementList } > > the structure is > > LabeledStatement > Identifier > : > NoScopeStatement > BlockStatement > { > StatementList > } > > No node that creates a scope here. It's the same way with the conditional > compilation statements. Of course, statements within the StatementList may > introduce their own scopes, but nothing in this parse tree as it stands creates > a scope. Hum. OK. I see how that makes sense. One of the things that trips me up though is that D created the possibility to label a block but doesn't do anything with it. To be honest, the way I had first understood labeling blocks was being able to do this: http://d.puremagic.com/issues/show_bug.cgi?id=8622 But even then, in that example, I would have expected a scope to be created.

(In reply to comment #22) > Hum. OK. I see how that makes sense. One of the things that trips > me up though is that D created the possibility to label a block but > doesn't do anything with it. The possibility to label a block is natural, because a block is a kind of statement. What's odd is that adding a label alone turns it into a scopeless block. > To be honest, the way I had first understood labeling blocks was being able to > do this: > http://d.puremagic.com/issues/show_bug.cgi?id=8622 I agree, that should work. But the way it is at the moment, people who try it are likely to be bitten by bug 827.

Thanks for clarifying... :) I can't see how this can be the intended behavior. C labels do not expand the following scope. They are simply anchors for goto and switch-case statements. D adds this cool feature of labeled scope, which has the unfortunate side effect of spilling the code of the block. I can't imagine how this has been the intention. I propose changing the spec like this: ScopeBlockOrNoScopeStatement: <-- new NoScopeStatement ScopeBlockStatement LabeledStatement: Identifier : ScopeBlockOrNoScopeStatement <-- use the new one NoScopeStatement: ; NonEmptyStatement <-- remove BlockStament to remove conflict with ScopeBlockStatement That leaves the only other user of NoScopeStatement in question: PragmaStatement: Pragma NoScopeStatement I am not sure how removing BlockStament from NoScopeStatement will affect PragmaStatement. Ali

(In reply to comment #24) > I propose changing the spec like this: > > ScopeBlockOrNoScopeStatement: <-- new > NoScopeStatement > ScopeBlockStatement > > LabeledStatement: > Identifier : ScopeBlockOrNoScopeStatement <-- use the new one How would all this differ from, let alone be an improvement over, simply changing LabeledStatement to LabeledStatement: Identifier : Statement ? > NoScopeStatement: > ; > NonEmptyStatement > <-- remove BlockStament to remove conflict with ScopeBlockStatement > > That leaves the only other user of NoScopeStatement in question: > > PragmaStatement: > Pragma NoScopeStatement What about conditional compliation statements (debug, version, static if)? These are the primary reason NoScopeStatement exists in the first place.

(In reply to comment #25) > How would all this differ from, let alone be an improvement over, simply > changing LabeledStatement to > > LabeledStatement: > Identifier : Statement Sounds good. :) > What about conditional compliation statements (debug, version, static if)? > These are the primary reason NoScopeStatement exists in the first place. I missed those because I made the mistake of searching only in the Statements page: http://dlang.org/statement.html Ali

(In reply to comment #16) > (In reply to comment #15) > > Walter Bright, comment #6: > > > I don't want to change this because it could break existing code > > > > We could deprecate having a BlockStatement after a label, with a message > > suggesting to move the label within the block. That way no code gets broken. > > Wait. What? Deprecate having a block statement after a label? Why would we do > that? That's completely arbitrary. Plus, that would *definitely* break more > code than what we'd break with a straight up fix. In order not to break code. I'm skeptical that a solution which causes any existing, debugged working code to silently break will be accepted, and I think that opinion makes sense. If we are to avoid all possible code breakage, it's simple to just deprecate non-empty block statements after a label, since there are two workarounds (see comment 15) which do not require changing existing scope semantics. The deprecation message can advise the user of the workarounds. I omitted to mention that an empty block statement after a label should still be allowed, as that is not bug-prone. Maybe I wasn't clear - by deprecating I mean permanent deprecation without removal, essentially a warning that you can turn off if you really want to by silencing deprecation messages. > This behavior was never according to spec anyways. I say we just fix it. It's a trade off between either breaking working code or annoying users with deprecation messages that need small fixes to their code.

(In reply to comment #27) > (In reply to comment #16) > > (In reply to comment #15) > > > Walter Bright, comment #6: > > > > I don't want to change this because it could break existing code > > > > > > We could deprecate having a BlockStatement after a label, with a message > > > suggesting to move the label within the block. That way no code gets broken. > > > > Wait. What? Deprecate having a block statement after a label? Why would we do > > that? That's completely arbitrary. Plus, that would *definitely* break more > > code than what we'd break with a straight up fix. > > In order not to break code. I'm skeptical that a solution which causes any > existing, debugged working code to silently break will be accepted, and I think > that opinion makes sense. > > If we are to avoid all possible code breakage, it's simple to just deprecate > non-empty block statements after a label, since there are two workarounds (see > comment 15) which do not require changing existing scope semantics. The > deprecation message can advise the user of the workarounds. > > I omitted to mention that an empty block statement after a label should still > be allowed, as that is not bug-prone. > > Maybe I wasn't clear - by deprecating I mean permanent deprecation without > removal, essentially a warning that you can turn off if you really want to by > silencing deprecation messages. > > > This behavior was never according to spec anyways. I say we just fix it. > > It's a trade off between either breaking working code or annoying users with > deprecation messages that need small fixes to their code. Just seems that because something is broken, your solution is to simply deprecate the entire feature. There is no reason to make labeling a block illegal or deprecated. The correct solution would be to find a path that doesn't break code, but still marks invalid code as such, and give users a pre-emptive chance to fix the code, before it is definitely banned. The (imo correct) path is that the NoScopeStatment should still provide no scope, but emit a deprecated message if anything declared in the scope is used. EG: void main() { label: { int i; } writeln(i); //(1) } (1) should emit a deprecation message.

(In reply to comment #28) > (In reply to comment #27) > > It's a trade off between either breaking working code or annoying users with > > deprecation messages that need small fixes to their code. > > Just seems that because something is broken, your solution is to simply > deprecate the entire feature. There is no reason to make labeling a block > illegal or deprecated. > > The correct solution would be to find a path that doesn't break code, but still > marks invalid code as such, and give users a pre-emptive chance to fix the > code, before it is definitely banned. I didn't advocate banning it. I advocate permanent deprecation, because the semantics are bug prone, and we should (arguably) never risk *silently* breaking working code. There is no reason to remove it for a long time, possibly never. > The (imo correct) path is that the NoScopeStatment should still provide no > scope, but emit a deprecated message if anything declared in the scope is used. > EG: > > void main() > { > label: > { > int i; > } > writeln(i); //(1) > } > > (1) should emit a deprecation message. Sounds good. Can it be implemented easily?

(In reply to comment #29) > (In reply to comment #28) > > (In reply to comment #27) > > > It's a trade off between either breaking working code or annoying users with > > > deprecation messages that need small fixes to their code. > > > > Just seems that because something is broken, your solution is to simply > > deprecate the entire feature. There is no reason to make labeling a block > > illegal or deprecated. > > > > The correct solution would be to find a path that doesn't break code, but still > > marks invalid code as such, and give users a pre-emptive chance to fix the > > code, before it is definitely banned. > > I didn't advocate banning it. I advocate permanent deprecation, because the > semantics are bug prone, and we should (arguably) never risk *silently* > breaking working code. There is no reason to remove it for a long time, > possibly never. I understand your position, but deprecation really implies that something WILL be removed, and usage NEEDS to stop. Having something permanently deprecated is not tenable: there is a compile mode that makes deprecated calls errors, and here, it is not an error. In particular, since there are builders out there that compile with "deprecation is error", well end up breaking *valid* code, when trying to avoid not breaking broken code :/ > > The (imo correct) path is that the NoScopeStatment should still provide no > > scope, but emit a deprecated message if anything declared in the scope is used. > > EG: > > > > void main() > > { > > label: > > { > > int i; > > } > > writeln(i); //(1) > > } > > > > (1) should emit a deprecation message. > > Sounds good. Can it be implemented easily? That is a very good question :D -------- Aren't we getting ahead of ourselves though? I think we should first concentrate on getting the spec changed, or at least, get Walter to agree that the current spec is not rational ? Well, maybe he can be better persuaded if we present a non-breaking fix...

(In reply to comment #29) > I didn't advocate banning it. I advocate permanent deprecation, > because the semantics are bug prone, and we should (arguably) never > risk *silently* breaking working code. Your worry about this is out of all proportion, when you consider that: (a) All the examples that have so far been posted here are contrived. Somebody please show me some real-world code that only works as the programmer intended because of this "feature". Surely there's far more real-world code that _doesn't_ behave as the programmer intended because of this. (b) D isn't even an ISO (or ANSI or whatever) standard yet. As such, it's still a _good_ time to get all the potentially breaking changes out of the way. (c) D has in its time had some breaking changes that are much more serious than this. (d) Even long-standardised programming languages have had breaking changes in their time. C has had at least one breaking change in its time, though I don't know when this came in relation to its standardisation. C++11 introduces a change to how template instantiations are lexed, which can cause code to behave differently. > There is no reason to remove it for a long time, > possibly never. > >> The (imo correct) path is that the NoScopeStatment should still >> provide no scope, but emit a deprecated message if anything >> declared in the scope is used. If you're going to go as far as that, why not emit a deprecation message if any declarations appear in it at all? >> EG: >> >> void main() >> { >> label: >> { >> int i; >> } >> writeln(i); //(1) >> } >> >> (1) should emit a deprecation message. > > Sounds good. Can it be implemented easily? I can see that it would be complicated to get it right. The tiny risk of silently breaking any real-world code doesn't seem to justify it. As such, IMM, just fixing this issue and being done with it is the way to go.

(In reply to comment #31) > >> The (imo correct) path is that the NoScopeStatment should still > >> provide no scope, but emit a deprecated message if anything > >> declared in the scope is used. > > If you're going to go as far as that, why not emit a deprecation message if any > declarations appear in it at all? That sounds easier to implement, and IMO is a bit better than deprecating having a non-empty block after a label. We would also need to deprecate having scope() statements in a labelled block. Bug 827 would also need a solution. comment #30: > I understand your position, but deprecation really implies that something WILL > be removed, and usage NEEDS to stop. Having something permanently deprecated is > not tenable: there is a compile mode that makes deprecated calls errors, and > here, it is not an error. If a user has chosen to enable deprecation errors, that is their problem IMO. By default the code would not break. Note: GTK2 kept its deprecated symbols until GTK3, rather than scheduling removal at a certain date. Anyway, a limited period of deprecation rather than just changing behaviour might be a good compromise. Perhaps 2 years?

(In reply to comment #32) > (In reply to comment #31) > > >> The (imo correct) path is that the NoScopeStatment should still > > >> provide no scope, but emit a deprecated message if anything > > >> declared in the scope is used. > > > > If you're going to go as far as that, why not emit a deprecation message if any > > declarations appear in it at all? > > That sounds easier to implement, and IMO is a bit better than deprecating > having a non-empty block after a label. We would also need to deprecate having > scope() statements in a labelled block. Bug 827 would also need a solution. Deprecating having a block after a label is wrong, and so is having a scope statement inside a labeled block. These are valid use cases of code, and both valid according to spec, old and new. Just because something is changing (more so, arguably being fixed), doesn't mean all previous usage of the code was wrong. > comment #30: > > I understand your position, but deprecation really implies that something WILL > > be removed, and usage NEEDS to stop. Having something permanently deprecated is > > not tenable: there is a compile mode that makes deprecated calls errors, and > > here, it is not an error. > > If a user has chosen to enable deprecation errors, that is their problem IMO. > By default the code would not break. The idea of deprecation is that something is removed, because that specific usage has been decided un-worthy, and usage should stop. This usually comes with an different/superior alternative. The error-on-deprecate are for users that want to keep their code up to date with the latest standard. This is not what we are doing at all. We changed a behavior, but there is nothing wrong with the code, and more importantly, there is no alternative to migrate to. > Note: GTK2 kept its deprecated symbols until GTK3, rather than scheduling > removal at a certain date. Yes, but they *were* scheduled for deprecation, and probably for a good reason. There is no good reason for a user to not use a labeled scope. > Anyway, a limited period of deprecation rather than just changing behaviour > might be a good compromise. Perhaps 2 years? I agree with Stewart, this all out of proportion. For starters, cases where code would actually break are minimal, and contrived. Not only that, but labels aren't something widely used anyways. In particular, I'd *highly* doubt that *anybody* actually explicitly relied on this behavior (or even knew about it...). If anything, the only changes in behavior I would expect are: a) Wrong code stops compiling (a good thing) b) Symbols weren't used anyways, no change c) User *expected* the end of the block to call a destroyer. The silently failing code would now be fixed. I really think the best solution here is just to fix the damn thing and move on.

The 9th commandment settles this IMO: "Where D code looks the same as C code, have it either behave the same or issue an error." Compiles without error, but fails in D. ============== int i = 1; void main() { label: { int i = 2; } assert(i == 1); } ============== I've been porting a fairly large C++ codebase to D (the D frontend) and this seems to be a fairly pointless gotcha. https://github.com/D-Programming-Language/dmd/pull/3024

Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/d0bfecae2ab724bb81b0b1ac5e093240420b6bde Fix Issue 199 - Label causes scope to collapse into parent https://github.com/D-Programming-Language/dmd/commit/ced168b8792cf47c0b1727b95019deec8f847627 Merge pull request #3024 from yebblies/issue199 Issue 199 - Label causes scope to collapse into parent

Reopening pending the necessary amendment to the spec. See comment 17.

The spec has already been updated. Open an enhancement if you want something else, this issue is fixed.

(In reply to comment #37) > The spec has already been updated. Open an enhancement if you want something > else, this issue is fixed. Where can I read a more up-to-date version of the spec than what is on dlang.org? And why has the 'spec' keyword been removed, even though the anomaly in the spec was part of the issue?

(In reply to comment #38) > Where can I read a more up-to-date version of the spec than what is on > dlang.org? On github: https://github.com/D-Programming-Language/dlang.org/pull/457 > And why has the 'spec' keyword been removed, even though the anomaly in the > spec was part of the issue? I don't know. I added it back in. This is also now reopened as a D1 only issue.

The docs over at: http://dlang.org/statement.html Still say: LabeledStatement: Identifier : Identifier : NoScopeStatement Identifier : Statement I believe this should be s/NoScopeStatement/ScopeBlockStatement/ ?

(In reply to monarchdodra from comment #40) > LabeledStatement: > Identifier : > Identifier : NoScopeStatement > Identifier : Statement > > I believe this should be s/NoScopeStatement/ScopeBlockStatement/ ? At the moment, it's completely ambiguous, because a ScopeBlockStatement is already a kind of Statement. As such, Identifier : NoScopeStatement should just be removed.

Closing as this bug is not valid in D2.