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.

Sign up for GitHub

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

Jump to bottom

Langversion for nameof feature #7125

Merged
merged 6 commits into from
Jul 4, 2019
Merged

Langversion for nameof feature #7125

merged 6 commits into from
Jul 4, 2019

Conversation

KevinRansom
Copy link
Member

  1. Enable LanguageVersion support for nameof.
  2. Validate errors.
  3. Update fsi build task to support langversion
  4. Test nameof on Fsi.
  5. Move tests from fsharp.core.unit tests to cambridge suite, since nameof isn't an fsharp.core feature

@dsyme can you check this out pls.

@KevinRansom KevinRansom requested a review from dsyme July 4, 2019 08:42
dsyme
dsyme approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

Just one question, but otherwise this looks great!

tests/fsharp/core/nameof/version46/test.fsx
@@ -0,0 +1,404 @@
//<Expects id="FS0039" status="error" span="(100,17)">The value or constructor 'nameof' is not defined.</Expects>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the error be "nameof requires /langversion:4.7 or greater?"

Copy link
Member Author

Choose a reason for hiding this comment

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

That is the original error message in older versions of the compiler.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need be this strict, but I'll leave it up to you and @cartermp. It feels like if we can program a better user experience easily we should.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dsyme, I agree, that we can improve things as we move forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should have been covered in the LangVersion RFC, but we need to emit a diagnostic about it not being available in the user's current LangVersion.

@dsyme
Copy link
Contributor

dsyme commented Jul 4, 2019

Here are the failures I spotted, though I don't understand how they are related to the PR

CodeGen\EmittedIL\CCtorDUWithMember (CCtorDUWithMember01.fs -) -- failed
CodeGen\EmittedIL\CCtorDUWithMember (CCtorDUWithMember04.fs) -- failed
CodeGen\EmittedIL\Misc (EntryPoint01.fs) -- failed
CodeGen\EmittedIL\Misc (ModuleWithExpression01.fs) -- failed
CodeGen\EmittedIL\QueryExpressionStepping (Linq101ElementOperators01.fs - CodeGen) -- failed
CodeGen\EmittedIL\StaticInit (LetBinding01.fs) -- failed
CodeGen\EmittedIL\TestFunctions (TestFunction9b4.fs) -- failed
CompilerOptions\fsc\staticlink (E_StaticLinkingErrorEXE.fs) -- failed
EntryPoint (W_NoEntryPointInLastModuleInsideMultipleNamespace.fs) -- failed
EntryPoint (W_NoEntryPointModuleInNamespace.fs) -- failed
EntryPoint (W_NoEntryPointMultipleModules.fs) -- failed
Libraries\Core\Unchecked (DefaultOf02) -- failed

dsyme
dsyme reviewed Jul 4, 2019
View reviewed changes
src/fsharp/NameResolution.fs Outdated
@@ -2475,7 +2476,15 @@ let rec ResolveExprLongIdentPrim sink (ncenv: NameResolver) first fullyQualified
| Exception e -> typeError := Some e; None

| true, res ->
Some (FreshenUnqualifiedItem ncenv m res, [])
let fresh = FreshenUnqualifiedItem ncenv m res, []
match fresh |> fst with
Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit calls to fst are not normally used if there is a chance to use a tuple pattern instead. S try

                let (item, _) as fresh = FreshenUnqualifiedItem ncenv m res, []
                match item with

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

Copy link
Member Author

Choose a reason for hiding this comment

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

I will look at the failures today, I have no idea why they occur.

src/fsharp/TastOps.fs Show resolved Hide resolved
@dsyme
Copy link
Contributor

dsyme commented Jul 4, 2019

OK this is ready now - @cartermp or @KevinRansom if you merge please it will unblock similar for #6806 and #6807

@KevinRansom
Copy link
Member Author

thx

@KevinRansom KevinRansom merged commit a7a66e3 into dotnet:release/fsharp47 Jul 4, 2019
@KevinRansom KevinRansom deleted the langversion-nameof branch July 10, 2019 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cartermp cartermp cartermp left review comments

@dsyme dsyme dsyme approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KevinRansom @dsyme @cartermp