Skip to content

Introduce warning for multiple entry points (sync + async)#46832

Merged
jcouv merged 33 commits intodotnet:masterfrom
Youssef1313:patch-7
Aug 26, 2020
Merged

Introduce warning for multiple entry points (sync + async)#46832
jcouv merged 33 commits intodotnet:masterfrom
Youssef1313:patch-7

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 commented Aug 14, 2020 •
edited
Loading

Fixes #46831

Summary

  • If one sync entry point found and one async entry point found => warning on async (new behavior)
  • If multiple sync entry points found => error on sync entry point (the same behavior as before), in addition to warnings on async entry points, if any (new behavior)
  • If multiple async entry points found but no sync entry points => error (the same behavior as before)
  • If multiple async entry points found and one synchronous entry point => warning (new behavior)

@Youssef1313 Youssef1313 requested a review from a team as a code owner August 14, 2020 17:40
@333fred
Copy link
Copy Markdown
Member

333fred commented Aug 14, 2020

@Youssef1313 I'm going to convert this to a draft. Feel free to convert back when it's ready.

@333fred 333fred marked this pull request as draft August 14, 2020 17:43
Youssef1313
Youssef1313 commented Aug 14, 2020
View reviewed changes
@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Aug 14, 2020

@RikkiGibson Can you have a look at this CI issue please?

Git fetch failed with exit code 128

@RikkiGibson
Copy link
Copy Markdown
Member

RikkiGibson commented Aug 14, 2020

Looks like an infra outage, let's retry the build in a few minutes. I think you can do this by closing+reopening your PR.

@Youssef1313 Youssef1313 reopened this Aug 14, 2020
@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Aug 14, 2020

Closing and reopening doesn't seem to restart the CI. I think it works only for employees. I'll make an empty commit now to retry.

Youssef1313
Youssef1313 commented Aug 15, 2020
View reviewed changes
@gafter
Copy link
Copy Markdown
Member

gafter commented Aug 15, 2020

I believe that for compatibility purposes, the diagnostic for this case should be a warning, and it should be produced only in a warning wave.

@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Aug 15, 2020

@gafter Wouldn't it be more convenient to introduce this as a breaking change?
I feel that multiple entry points is an error more than just a warning, and this behavior will be consistent with multiple synchronous entry points.
Why should the behavior for multiple entry points containing an async main be different than multiple entry points that are all sync?

I also would like to know more info on the warning wave thing, and how its process goes.

@333fred
Copy link
Copy Markdown
Member

333fred commented Aug 17, 2020 •
edited
Loading

gafter Wouldn't it be more convenient to introduce this as a breaking change?
I feel that multiple entry points is an error more than just a warning, and this behavior will be consistent with multiple synchronous entry points.
Why should the behavior for multiple entry points containing an async main be different than multiple entry points that are all sync?

I also would like to know more info on the warning wave thing, and how its process goes.

This isn't really an option. We take backcompat seriously, and introducing a new warning that cannot be opted out of is absolutely a backcompat break.

You'll need to add the warning to the level 5 switch here: http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Errors/ErrorFacts.cs,210, and make sure to follow the WRN prefix instead of an ERR prefix.

@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Aug 17, 2020

@333fred Thanks. I'll change it to a warning. But I still need to know where my implementation went wrong. Thanks in advance.

@Youssef1313 Youssef1313 changed the title WIP: Fix multiple entry points WIP: Introduce warning for multiple entry points (sync + async) Aug 17, 2020
@Youssef1313 Youssef1313 marked this pull request as ready for review August 17, 2020 12:21
@Youssef1313 Youssef1313 requested review from 333fred and removed request for a team August 17, 2020 12:21
333fred
333fred reviewed Aug 17, 2020
View reviewed changes
RikkiGibson
RikkiGibson reviewed Aug 19, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

Looks good, just had a few minor comments

@RikkiGibson RikkiGibson added the Feature - Warning Waves Warning Waves label Aug 19, 2020
@333fred
Copy link
Copy Markdown
Member

333fred commented Aug 19, 2020

This is looking very close. I think Rikki's couple of comments and then it'll be good.

@333fred
Copy link
Copy Markdown
Member

333fred commented Aug 19, 2020

Oh, and we need to add this warning to the list here: https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/Warnversion%20Warning%20Waves.md

333fred
333fred approved these changes Aug 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 33). @jaredpar, please review as this adds a new warning wave warning.

@333fred 333fred requested a review from jaredpar August 20, 2020 17:31
@RikkiGibson RikkiGibson added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Aug 20, 2020
RikkiGibson
RikkiGibson approved these changes Aug 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Aug 20, 2020

@RikkiGibson @333fred Thanks for the help you provided too! ❤️

@Youssef1313 Youssef1313 marked this pull request as draft August 23, 2020 05:03
@Youssef1313 Youssef1313 marked this pull request as ready for review August 23, 2020 05:04
@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Aug 23, 2020

@RikkiGibson @333fred Should I restart CI on this one?

Pinging @jaredpar for review.

@RikkiGibson
Copy link
Copy Markdown
Member

RikkiGibson commented Aug 23, 2020

Have rekicked the failing CI leg.

@333fred
Copy link
Copy Markdown
Member

333fred commented Aug 25, 2020

@jcouv or @cston can one of you two review this change please?

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Aug 26, 2020

Looking

jcouv
jcouv reviewed Aug 26, 2020
View reviewed changes
jcouv
jcouv approved these changes Aug 26, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 33)

@jcouv jcouv self-assigned this Aug 26, 2020
@jcouv jcouv merged commit 601cd9a into dotnet:master Aug 26, 2020
@ghost ghost added this to the Next milestone Aug 26, 2020
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Aug 26, 2020

Merged/squashed. Thanks @Youssef1313 !

@Youssef1313 Youssef1313 deleted the patch-7 branch August 26, 2020 21:05
333fred added a commit to 333fred/roslyn that referenced this pull request Aug 27, 2020
@333fred
Merge remote-tracking branch 'upstream/master' into unmanaged-callers…
a25ef2d 
…-only-errors

* upstream/master: (236 commits)
  Fix bug when "End statement" is used in single-line if (dotnet#47062)
  Solution asset cache refactoring (dotnet#46948)
  add specific tests to validate behavior between keys and snapshots
  Extract into separate files
  rename parameters
  rename parameters
  rename parameters
  rename parameters
  Add CancellationToken parameters to SyntaxTreeOptionsProvider
  Reuse nullable override checks for delegate conversions (dotnet#46953)
  Introduce warning for multiple entry points (sync + async) (dotnet#46832)
  Switch from throwing NotImplementedException and return E_NOTIMPL
  Delete Building for Core CLR.md (dotnet#47146)
  Adjust PrintMembers to avoid boxing and avoid extra space (dotnet#47095)
  Track asynchronous operation in InProcLanguageServer
  Use Task.FromCanceled where appropriate
  Apply suggestions from code review
  Address feedback
  Expose ParseOptions on generator context (dotnet#46919)
  Remove redundant statement in added tests
  ...
@allisonchou allisonchou modified the milestones: Next, 16.8.P3 Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

@jaredpar jaredpar Awaiting requested review from jaredpar

Assignees

@jcouv jcouv

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature - Warning Waves Warning Waves

Projects

None yet

Milestone

16.8.P3

Development

Successfully merging this pull request may close these issues.

CS0017 (multiple entry point) isn't always emitted

6 participants

@Youssef1313 @333fred @RikkiGibson @gafter @jcouv @allisonchou