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.

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

Allow comments after explicit line continuations #65

Closed
Echo-8-ERA opened this issue Apr 24, 2017 · 41 comments
Closed

Allow comments after explicit line continuations #65

Echo-8-ERA opened this issue Apr 24, 2017 · 41 comments
Assignees
Labels
Approved-in-Principle LDM Considering LDM has reviewed and we think this has merit Proposal

Comments

@Echo-8-ERA
Copy link

Currently, VB allows the use of comments after implicit line continuations:

foo(a.
      b(c).    ' Comment 1
      d(e, f), ' Comment 2
    g(h))      ' Comment 3

However, it doesn't allow comments after explicit ones:

' error BC30999: Line continuation character '_' must be preceded by at least one white space and must be the last character on the line.
foo(a _
      .b(c) _   ' Comment 1
      .d(e, f), ' Comment 2
    g(h))       ' Comment 3

Personally, I greatly prefer having the operator at the beginning of a line, as it is an additional visual cue that I'm chaining method calls or writing a very long expression, as opposed to breaking up a very long method call into separate lines for each argument.

@AdamSpeight2008
Copy link
Contributor

See related: #41

@AnthonyDGreen
Copy link
Contributor

It looks terrifying but we still want it. I believe Lucian sent an email saying he wanted to be able to write the text of war and peace in the margins of his code.

@paul1956
Copy link

It's been over a year, what does Approved-in-Principle actually mean? Is any work being done to increase where I can put comments? There are several Open issues around comments and some are non-breaking. Maybe there is a way to open up a discussion around non-breaking ways to add comments (and blank lines) in more places. I saw that changing where comments are attached was closed but allowing comments after an _ could serve the same purpose and would be non-breaking.

@gafter
Copy link
Member

gafter commented Jun 30, 2018

@paul1956 approved in principle means the LDM thinks it is an idea worth pursuing and likely doing. I am not aware of anyone in particular doing anything to make it happen (including you nor me). This is the right place to discuss it. If you want to move it forward I recommend you announce that you’re going to work on a prototype and do so (as a PR in the Roslyn repo).

@paul1956
Copy link

paul1956 commented Jul 1, 2018

@gafter as I have stated before I don't know enough about GitHub, a PR or where even to find the actual VB compiler to attempt it. If someone wants to point to the parser source or a place to start I would be very interested in working on it. I still see Roslyn as a very big black box with source, and developer rules/controls but not a lot of instruction on what to do with it. As compared to the Analyzers and Refactoring's which have well documented examples and words/whitepapers that explain the concepts. I have just completed a C# to VB translator 99% in VB that can convert all of Roslyn to VB, parse without errors, and compile and I only comment out a few things that have no way to be represented in VB (most Unsafe code), I still need to address nested Modules, ERRID.ERR_ModuleNotAtNamespace, so for now I ignore it and ERRID.ERR_StandaloneAttribute), and I preserve all comments and most formatting, but some comments are so far from where they belong they are useless or misleading. It is these comments and ERRID.ERR_StandaloneAttribute I want to address.

@gafter
Copy link
Member

gafter commented Jul 1, 2018

The VB scanner is HERE and the parser is HERE. We welcome contributions. I expect someone or other will undertake this project sometime over the next 5-6 years, but if it is important to you, then you might be the best person to do it.

@paul1956
Copy link

paul1956 commented Jul 1, 2018

@gafter, thanks I will look at it. Cloning, restoring and building Roslyn is a lot easier than the last time I tried. Any place to find tips on building the VB Compiler and testing my changes as well as any design documents/whitepapers for first time developers?

@paul1956
Copy link

paul1956 commented Jul 2, 2018

@gafter is there a preference on Syntax I want to tackle this case first and then do _ comment on a blank line

Dim I As Integer = _'This is a comment with ' immediately following
1

Or

Dim I As Integer = _ ' This is a comment with required leading space
1

@Echo-8-ERA
Copy link
Author

I personally think the version with a space is better. Reduces the possibility of confusing it with an identifier.

@reduckted
Copy link
Contributor

The IDE adds a space before comment markers anyway, so the second option is the most natural.
foo

@paul1956
Copy link

paul1956 commented Jul 3, 2018

Space wins. As it turns out this is "trivial", the entire change is about 25 lines in scanner.vb, about 6 new lines and a few lines moved around and 2 new tests need to be added. All the infrastructure already assume this was a feature.

My next question is how to write a PR. I am using VS 2017 enterprise and did use GitHub to add the project and my changes show up with red Checks next to them.

@paul1956
Copy link

paul1956 commented Jul 3, 2018

@reduckted I have read all that before but none of it really help someone who has never done it before. To start I cloned Master, do I now branch or make the changed then branch? I have saved the changes on my local system and reverted back to master. I don't even know what Jenkins is, what I really need is to get to "feature is championed and validated by LDM a developer will be assigned to help". I am sure once I get through the first one, other should be easy.

@gafter gafter self-assigned this Jul 3, 2018
@gafter
Copy link
Member

gafter commented Jul 3, 2018

As a member of the LDM, I hereby champion this proposal. @jaredpar We should assign someone to help this external developer, please.

You should work on a "branch". If you're using the git command-line, you create a branch locally by doing something like

git checkout -b vblang-65

where vblang-65 is whatever you want your branch to be named (that would be a reasonable name for the branch). Then you need to push that branch to your own github clone of the Roslyn repository. I do that something like this

git push myrepository vblang-65

Once you've done that you're ready to submit a pull request for us to review.

@paul1956
Copy link

paul1956 commented Jul 3, 2018

I am using Visual Studio 2017 Enterprise and have Roslyn cloned on my local system where I have made and tested the change, backed up and undid the 2 changed files.

How does the "git checkout" know what and where it is checking out from? After I branch how do I tell VS to open my branch instead of the original.

Sorry for what seems like silly questions but it is been decades since I worked on anything as part of a team and then we used Visual Safe Source.

@KathleenDollard
Copy link
Contributor

This is a great opportunity for community!!!

@AdamSpeight2008 or @AnthonyDGreen or anyone else.

If you can walk @paul1956 through the process and create any docs along the way (for example, I always work on a fork, not just a branch, and what the heck is the difference). It is an intimidating process.

(As an aside, some people have their mom call about how to fix the DVR or some email feature. My kids still get a call when I have a pending "git-astophy" - git catostrphy)

To me, part of what it means to have a community here is for people to help each other working with our git and our systems.

@AdamSpeight2008
Copy link
Contributor

@paul1956 I personally use TortoiseGit, than Visual Studio for git stuff mostly along with VS Command Prompt.
I've multiple repositories.

  • Roslyn the official repo.
  • Roslyn-AdamSpeight2008 my fork.
    This is so I can work locally on stuff with out having internet connectivity, as well as do and try stuff with out affect other things. Always create a new branch when trying thing out, as they're cheap and can discarded at later date. As treat these as a kind of scratch pad.
  • VB Lang So I can add documentation, or experiment with changes to documentation.

Adding a Feature (unofficial)

  • Fork Roslyn
    • Note: You may also need to white list / except some roslyn folder from being monitor by your antivirus software.
  • Git Fetch All To get the latest implemented build of all the branches.
  • Create a new branch of from master
    • Targeting master is a good initial target, unless adviced otherwise.
    • Use a branch name that covers what your modifications intent.
    • I also a to add a quick description / note, so future me can remember what this branch does.
  • Open Visual Studio Command Line
    • Personally mine is elevated.
  • cd /d D:\roslyn-AdamSpeight2008 This is my local repo
  • git clean -xdf This is clear the repo of crud left-behind from a switch of branch
  • restore This should pull in the versions of the subsystem needed to build your current branch state
  • build Ensure it builds before any changes
  • test Ensure it passes the unittests before any changes
    This ensure that you are starting from a known good state, and any build errors or failing unit test are highly likely down to something you've done.
* You may want to do a initial `git commit` here.

@paul1956
Copy link

paul1956 commented Jul 5, 2018

@AdamSpeight2008 if we could start simple, I have forked Roslyn by clicking Fork Icon on GitHub/Roslyn website and now I have paul1956\Roslyn I don't see any way to add my name after Roslyn on the website. Then I cloned it to my local machine using Open in Visual Studio where I was given an opportunity to change name. Git Fetch All says 'All' Does not appear to be a repository. My fork only has 15 files/directories in the root while Master has 26, all the CMD files are missing as is Compiler.SLN (the thing I am working on)

@AdamSpeight2008
Copy link
Contributor

@paul1956
Copy link

paul1956 commented Jul 6, 2018

@AdamSpeight2008 After cloning, my Roslyn looks nothing like the original, restore, build and test commands don't exist. Watching the video was interesting but didn't explain why my fork looks nothing like Roslyn.
image. I have manually committed the 3 files (video helped) and did a pull request. I will wait to see how it goes, what I have done till now would not work for more significant changes.

@AdamSpeight2008
Copy link
Contributor

@paul1956 Looks way out of date, 3 years at least.

@jmarolf
Copy link

jmarolf commented Jul 6, 2018

@paul1956 you will need to pull in the latest changes from the main roslyn branch into your fork.

@paul1956
Copy link

paul1956 commented Jul 7, 2018

@jmarolf I did a fork yesterday from https://github.com/dotnet/roslyn and the image above is what I got. I have been told to pull lastest or rebase. I did the first from my PC to my local Repo and nothing changed, and Google and Video don't help with how to Rebase. I did the development by downloading a zip file with Visual Studio from https://github.com/dotnet/roslyn and then Forked Roslyn and said open in Visual Studio, my fork does appear 3 years old but my Zip is current, this doesn't seem like my issue. Exactly how to I pull in latest changes into https://github.com/paul1956/Roslyn or how do I rebase?

@paul1956
Copy link

paul1956 commented Jul 7, 2018

@AdamSpeight2008 the image is from a fork I did yesterday.

@AdamSpeight2008
Copy link
Contributor

@AnthonyDGreen @KathleenDollard @paul1956
Question: Does this also permit comments using REM comment comments?

@KathleenDollard
Copy link
Contributor

Thank you @paul1956 and @AdamSpeight2008!

I think not on the REM comments. I could be talked out of that position if there are compelling arguments for it. "We might as well" doesn't seem a compelling argument and it's the only one I have thought of so far.

@AdamSpeight2008
Copy link
Contributor

@KathleenDollard I'm only asking as the VB Grammar, permits both as a CommentMarker.

Comment
    : CommentMarker Character*
    ;

CommentMarker
    : SingleQuoteCharacter
    | 'REM'
    ;

SingleQuoteCharacter
    : '\''
    | '<Unicode 0x2018>'
    | '<Unicode 0x2019>'
    ;

@paul1956
Copy link

@AdamSpeight2008 Right now I am only working on _ ' Comment. I did not know REM existed and after I saw that it did I tried to use it to address my original issue of being able to put comments in more places and unfortunately Rem comments didn't solve my issues (I was hoping I could put a REM on a blank line and have it ignored but that is not the case. I hope eventually _ ' Comment on a blank line allow it to be ignored but this may take a lot of work. I use ' but the code works with any SingleQuoteCharacter.

@franzalex
Copy link

I have only ever thought of REM as a keyword which causes the entire line to be interpreted as a comment. Going by that reasoning, I don't think we ought to allow it's usage in this scenario.


PS: In all my 15 years of writing VB code, I've never commented with REM. Neither have I met code originally written in VB.NET that uses REM as the comment marker.

@gafter gafter added the LDM Considering LDM has reviewed and we think this has merit label Jul 15, 2018
@AnthonyDGreen
Copy link
Contributor

The implementation doesn't distinguish ' from REM as much as people think. Here's (one place) where it happens. I remember there being pushback one MVP summit about the Roslyn team having the wrong priorities for VB because REM was supported very early while other "statements" were not because of a technical misunderstanding. REM isn't really a "keyword" any more than ' is an "operator", it just marks the start of a comment which all happens during lexical analysis before parsing or any semantics and is never really looked at again (it's trivia, not even a "statement"). It would be more work to treat REM specially.

On the practical side, I do use REM but only for a very specific situation: When I'm reviewing someone else's code, typically reviewing documentation or VB code samples for other teams I use REM comments for my comments (meta-comments so to speak) to distinguish from comments that are actually part of the sample or documentation. They stick out visually specifically because no one uses them and they shouldn't be present in the shipping sample code. Makes them crazy easy to see and search for with both the eyes and tools.

@paul1956
Copy link

paul1956 commented Aug 2, 2018

Right now Rem is not allowed after an _. What is allowed is _ followed by 0 or more spaces followed by ', then everything till the end of line is a comment. Adding Rem would be possible but would require a lot of additional tests and some changes to the scanner.

@AnthonyDGreen
Copy link
Contributor

Oops. I confused the suggestion. Thought this was modifying an existing case with implicit line continuation for some reason (maybe because I was reading #232 at the same time). This is a completely new case, so the existing code paths don't really matter. I would guess that it would mostly just double the number of new tests replacing ' with REM but there might be more stuff around expand/reduce in the IDE I'm not thinking about--I legit don't know.

Random thoughts on REM:

  • By enabling comments after explicit line continuation I think you're making it such there everywhere you could have an implicit line continuation you could replace it with an explicit one and not break code; comments after implicit line continuation broke this. Interchangeability is good.
  • By not enabling REM you're now introducing a new place where ' and REM can't be freely interchanged, where before this was rare. Might not matter though.
  • What'll be interesting is if another proposal allows code like this:
Dim q = From x
        ' Comment
        In y
        REM Comment
        Select x

This was more the scenario I think I was thinking about before. I suspect it'll use more of the existing codepaths for scanning (just a guess). Then it would be weird if REM just works in some new cases but not others just based on whether existing code was reused or not.

  • I'd prefer not to just let parts of the language bit-rot that are out of vogue because fashions change but don't have much else compelling.

@AnthonyDGreen
Copy link
Contributor

If the VB LDM would allow it, I'd be happy to submit support for REM in a separate follow-up PR, so as not to hold this one up.

@paul1956
Copy link

paul1956 commented Aug 3, 2018

@AnthonyDGreen I would love to allow comments on any "blank Lines" with Explicit or Implicit line continuations C# already allows this. I have suggested that several different ways. Allowing Rem after _ ' is something completely different and I can look at it or you can but I though @KathleenDollard did not see the value and I agreed.

@paul1956
Copy link

@AnthonyDGreen I looked at supporting Rem, I thought is would be a very trivial change just replace IsSingleQuote(Peek(Here)) with PeekStartComment(Here) > 0 but several tests fail beyond just the tests that were expected to fail and now pass and multiple test support routines that "rem" tests use don't support passing in a compiler version.

@jcouv
Copy link
Member

jcouv commented Sep 20, 2018

@KathleenDollard It would be good to create a 16.0 milestone and attach this feature, since @paul1956 is working on implementation. Thanks
Another feature worth recording is a relaxation in conditional expressions to allow unconstrained types.

@jcouv
Copy link
Member

jcouv commented Apr 2, 2019

The feature was merged for dev16.1 preview2 (dotnet/roslyn#28327) which includes a preview of VB 16.

@KathleenDollard
Copy link
Contributor

I am closing this, since this feature is now merged and scheduled for the next update to Visual Studio 2019, which is now in Preview.

It has been a long process and massive thanks to @paul1956 for this work!!!

@tverweij
Copy link

Solved in project Mercury

@markhealy88
Copy link

Has this regressed in 16.5.4?

@paul1956
Copy link

I don't think so but some installations like Roslyn explicitly set VB version to 15.5 in a settings file and some project files also do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved-in-Principle LDM Considering LDM has reviewed and we think this has merit Proposal
Projects
None yet
Development

No branches or pull requests