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

Always emit conditional attributes in FCS #6004

Merged
merged 6 commits into from
Jan 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/fsharp/CompileOps.fs
Original file line number Diff line number Diff line change
Expand Up @@ -2356,6 +2356,8 @@ type TcConfigBuilder =
mutable tryGetMetadataSnapshot : ILReaderTryGetMetadataSnapshot

mutable internalTestSpanStackReferring : bool

mutable noConditionalErasure : bool
}

static member Initial =
Expand Down Expand Up @@ -2493,6 +2495,7 @@ type TcConfigBuilder =
shadowCopyReferences = false
tryGetMetadataSnapshot = (fun _ -> None)
internalTestSpanStackReferring = false
noConditionalErasure = false
}

static member CreateNew(legacyReferenceResolver, defaultFSharpBinariesDir, reduceMemoryUsage, implicitIncludeDir,
Expand Down Expand Up @@ -2954,6 +2957,8 @@ type TcConfig private (data : TcConfigBuilder, validate:bool) =
member x.shadowCopyReferences = data.shadowCopyReferences
member x.tryGetMetadataSnapshot = data.tryGetMetadataSnapshot
member x.internalTestSpanStackReferring = data.internalTestSpanStackReferring
member x.noConditionalErasure = data.noConditionalErasure

static member Create(builder, validate) =
use unwindBuildPhase = PushThreadBuildPhaseUntilUnwind BuildPhase.Parameter
TcConfig(builder, validate)
Expand Down Expand Up @@ -5440,9 +5445,12 @@ let TypeCheckOneInputEventually (checkForErrors, tcConfig:TcConfig, tcImports:Tc
if Zset.contains qualNameOfFile tcState.tcsRootImpls then
errorR(Error(FSComp.SR.buildImplementationAlreadyGivenDetail(qualNameOfFile.Text), m))

let conditionalDefines =
if tcConfig.noConditionalErasure then None else Some (tcConfig.conditionalCompilationDefines)

// Typecheck the signature file
let! (tcEnv, sigFileType, createsGeneratedProvidedTypes) =
TypeCheckOneSigFile (tcGlobals, tcState.tcsNiceNameGen, amap, tcState.tcsCcu, checkForErrors, tcConfig.conditionalCompilationDefines, tcSink, tcConfig.internalTestSpanStackReferring) tcState.tcsTcSigEnv file
TypeCheckOneSigFile (tcGlobals, tcState.tcsNiceNameGen, amap, tcState.tcsCcu, checkForErrors, conditionalDefines, tcSink, tcConfig.internalTestSpanStackReferring) tcState.tcsTcSigEnv file

let rootSigs = Zmap.add qualNameOfFile sigFileType tcState.tcsRootSigs

Expand Down Expand Up @@ -5477,9 +5485,12 @@ let TypeCheckOneInputEventually (checkForErrors, tcConfig:TcConfig, tcImports:Tc

let tcImplEnv = tcState.tcsTcImplEnv

let conditionalDefines =
if tcConfig.noConditionalErasure then None else Some (tcConfig.conditionalCompilationDefines)

// Typecheck the implementation file
let! topAttrs, implFile, _implFileHiddenType, tcEnvAtEnd, createsGeneratedProvidedTypes =
TypeCheckOneImplFile (tcGlobals, tcState.tcsNiceNameGen, amap, tcState.tcsCcu, checkForErrors, tcConfig.conditionalCompilationDefines, tcSink, tcConfig.internalTestSpanStackReferring) tcImplEnv rootSigOpt file
TypeCheckOneImplFile (tcGlobals, tcState.tcsNiceNameGen, amap, tcState.tcsCcu, checkForErrors, conditionalDefines, tcSink, tcConfig.internalTestSpanStackReferring) tcImplEnv rootSigOpt file

let hadSig = rootSigOpt.IsSome
let implFileSigType = SigTypeOfImplFile implFile
Expand Down
3 changes: 3 additions & 0 deletions src/fsharp/CompileOps.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,9 @@ type TcConfigBuilder =

/// if true - 'let mutable x = Span.Empty', the value 'x' is a stack referring span. Used for internal testing purposes only until we get true stack spans.
mutable internalTestSpanStackReferring : bool

/// Prevent erasure of conditional attributes and methods so tooling is able analyse them.
mutable noConditionalErasure: bool
}

static member Initial: TcConfigBuilder
Expand Down
9 changes: 5 additions & 4 deletions src/fsharp/CompileOptions.fs
Original file line number Diff line number Diff line change
Expand Up @@ -848,16 +848,17 @@ let testFlag tcConfigB =
| str -> warning(Error(FSComp.SR.optsUnknownArgumentToTheTestSwitch(str),rangeCmdArgs))), None,
None)

// not shown in fsc.exe help, no warning on use, motivation is for use from VS
let vsSpecificFlags (tcConfigB: TcConfigBuilder) =
// Not shown in fsc.exe help, no warning on use, motivation is for use from tooling.
let editorSpecificFlags (tcConfigB: TcConfigBuilder) =
[ CompilerOption("vserrors", tagNone, OptionUnit (fun () -> tcConfigB.errorStyle <- ErrorStyle.VSErrors), None, None)
CompilerOption("validate-type-providers", tagNone, OptionUnit (id), None, None) // preserved for compatibility's sake, no longer has any effect
CompilerOption("LCID", tagInt, OptionInt ignore, None, None)
CompilerOption("flaterrors", tagNone, OptionUnit (fun () -> tcConfigB.flatErrors <- true), None, None)
CompilerOption("sqmsessionguid", tagNone, OptionString ignore, None, None)
CompilerOption("gccerrors", tagNone, OptionUnit (fun () -> tcConfigB.errorStyle <- ErrorStyle.GccErrors), None, None)
CompilerOption("exename", tagNone, OptionString (fun s -> tcConfigB.exename <- Some(s)), None, None)
CompilerOption("maxerrors", tagInt, OptionInt (fun n -> tcConfigB.maxErrors <- n), None, None) ]
CompilerOption("maxerrors", tagInt, OptionInt (fun n -> tcConfigB.maxErrors <- n), None, None)
CompilerOption("noconditionalerasure", tagNone, OptionUnit (fun () -> tcConfigB.noConditionalErasure <- true), None, None) ]

let internalFlags (tcConfigB:TcConfigBuilder) =
[
Expand Down Expand Up @@ -896,7 +897,7 @@ let internalFlags (tcConfigB:TcConfigBuilder) =
CompilerOption("alwayscallvirt",tagNone,OptionSwitch(callVirtSwitch tcConfigB),Some(InternalCommandLineOption("alwayscallvirt",rangeCmdArgs)), None)
CompilerOption("nodebugdata",tagNone, OptionUnit (fun () -> tcConfigB.noDebugData<-true),Some(InternalCommandLineOption("--nodebugdata",rangeCmdArgs)), None)
testFlag tcConfigB ] @
vsSpecificFlags tcConfigB @
editorSpecificFlags tcConfigB @
[ CompilerOption("jit", tagNone, OptionSwitch (jitoptimizeSwitch tcConfigB), Some(InternalCommandLineOption("jit", rangeCmdArgs)), None)
CompilerOption("localoptimize", tagNone, OptionSwitch(localoptimizeSwitch tcConfigB),Some(InternalCommandLineOption("localoptimize", rangeCmdArgs)), None)
CompilerOption("splitting", tagNone, OptionSwitch(splittingSwitch tcConfigB),Some(InternalCommandLineOption("splitting", rangeCmdArgs)), None)
Expand Down
13 changes: 6 additions & 7 deletions src/fsharp/TypeChecker.fs
Original file line number Diff line number Diff line change
Expand Up @@ -504,8 +504,8 @@ type cenv =
/// Used to resolve names
nameResolver: NameResolver

/// The set of active conditional defines
conditionalDefines: string list
/// The set of active conditional defines. The value is None when conditional erasure is disabled in tooling.
conditionalDefines: string list option
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of list option types as I usually get confused by what it could mean. The comment does help though, so it's not a mystery. Though, I prefer another bool field honestly. What you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think I'm okay with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you ok with it? (Just curious because I'm on the fence).

The more I think about it, the more I really want a new bool field i.e isConditionalErasureEnabled : bool. Nothing can be clearer than that especially when it's used in code.

Using an option gives the field conditionalDefines more responsibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

@TIHan On the other hand using option makes one aware of situation when there may be no conditional erasure unlike a separate field that one may be unaware of.


isInternalTestSpanStackReferring: bool
}
Expand Down Expand Up @@ -3004,8 +3004,8 @@ let BuildPossiblyConditionalMethodCall cenv env isMutable m isProp minfo valUseF

let conditionalCallDefineOpt = TryFindMethInfoStringAttribute cenv.g m cenv.g.attrib_ConditionalAttribute minfo

match conditionalCallDefineOpt with
| Some d when not (List.contains d cenv.conditionalDefines) ->
match conditionalCallDefineOpt, cenv.conditionalDefines with
| Some d, Some defines when not (List.contains d defines) ->

// Methods marked with 'Conditional' must return 'unit'
UnifyTypes cenv env m cenv.g.unit_ty (minfo.GetFSharpReturnTy(cenv.amap, m, minst))
Expand Down Expand Up @@ -10828,11 +10828,10 @@ and TcAttribute canFail cenv (env: TcEnv) attrTgt (synAttr: SynAttribute) =

let conditionalCallDefineOpt = TryFindTyconRefStringAttribute cenv.g mAttr cenv.g.attrib_ConditionalAttribute tcref

match conditionalCallDefineOpt with
| Some d when not (List.contains d cenv.conditionalDefines) ->
match conditionalCallDefineOpt, cenv.conditionalDefines with
| Some d, Some defines when not (List.contains d defines) ->
[], false
| _ ->

// REVIEW: take notice of inherited?
let validOn, _inherited =
let validOnDefault = 0x7fff
Expand Down
4 changes: 2 additions & 2 deletions src/fsharp/TypeChecker.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@ val EmptyTopAttrs : TopAttribs
val CombineTopAttrs : TopAttribs -> TopAttribs -> TopAttribs

val TypeCheckOneImplFile :
TcGlobals * NiceNameGenerator * ImportMap * CcuThunk * (unit -> bool) * ConditionalDefines * NameResolution.TcResultsSink * bool
TcGlobals * NiceNameGenerator * ImportMap * CcuThunk * (unit -> bool) * ConditionalDefines option * NameResolution.TcResultsSink * bool
-> TcEnv
-> Tast.ModuleOrNamespaceType option
-> ParsedImplFileInput
-> Eventually<TopAttribs * Tast.TypedImplFile * ModuleOrNamespaceType * TcEnv * bool>

val TypeCheckOneSigFile :
TcGlobals * NiceNameGenerator * ImportMap * CcuThunk * (unit -> bool) * ConditionalDefines * NameResolution.TcResultsSink * bool
TcGlobals * NiceNameGenerator * ImportMap * CcuThunk * (unit -> bool) * ConditionalDefines option * NameResolution.TcResultsSink * bool
-> TcEnv
-> ParsedSigFileInput
-> Eventually<TcEnv * ModuleOrNamespaceType * bool>
Expand Down
25 changes: 24 additions & 1 deletion tests/service/Symbols.fs
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,27 @@ module Mod2 =
mod1val1.XmlDocSig |> shouldEqual "P:Mod1.val1"
mod2func2.XmlDocSig |> shouldEqual "M:Mod1.Mod2.func2"



module Attributes =
[<Test>]
let ``Emit conditional attributes`` () =
let source = """
open System
open System.Diagnostics

[<Conditional("Bar")>]
type FooAttribute() =
inherit Attribute()

[<Foo>]
let x = 123
"""
let fileName, options = mkTestFileAndOptions source [| "--noconditionalerasure" |]
let _, checkResults = parseAndCheckFile fileName source options

checkResults.GetAllUsesOfAllSymbolsInFile()
|> Async.RunSynchronously
|> Array.tryFind (fun su -> su.Symbol.DisplayName = "x")
|> Option.orElseWith (fun _ -> failwith "Could not get symbol")
|> Option.map (fun su -> su.Symbol :?> FSharpMemberOrFunctionOrValue)
|> Option.iter (fun symbol -> symbol.Attributes.Count |> shouldEqual 1)