Skip to content

discussion: naming fields in ConstraintSolver's exception types #6604

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

Merged
KevinRansom merged 1 commit into from
Apr 24, 2019
Merged

discussion: naming fields in ConstraintSolver's exception types #6604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/fsharp/CompileOps.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ let OutputPhasedErrorR (os: StringBuilder) (err: PhasedDiagnostic) (suggestNames
if m.StartLine <> m2.StartLine then
os.Append(SeeAlsoE().Format (stringOfRange m)) |> ignore

| ConstraintSolverInfiniteTypes(contextInfo, denv, t1, t2, m, m2) ->
| ConstraintSolverInfiniteTypes(denv, contextInfo, t1, t2, m, m2) ->
// REVIEW: consider if we need to show _cxs (the type parameter constraints)
let t1, t2, _cxs = NicePrint.minimalStringsOfTwoTypes denv t1 t2
os.Append(ConstraintSolverInfiniteTypesE().Format t1 t2) |> ignore
Expand Down
34 changes: 17 additions & 17 deletions src/fsharp/ConstraintSolver.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,21 +155,21 @@ type ContextInfo =
/// The type equation comes from a sequence expression.
| SequenceExpression of TType

exception ConstraintSolverTupleDiffLengths of DisplayEnv * TType list * TType list * range * range
exception ConstraintSolverInfiniteTypes of ContextInfo * DisplayEnv * TType * TType * range * range
exception ConstraintSolverTypesNotInEqualityRelation of DisplayEnv * TType * TType * range * range * ContextInfo
exception ConstraintSolverTypesNotInSubsumptionRelation of DisplayEnv * TType * TType * range * range
exception ConstraintSolverMissingConstraint of DisplayEnv * Tast.Typar * Tast.TyparConstraint * range * range
exception ConstraintSolverError of string * range * range
exception ConstraintSolverRelatedInformation of string option * range * exn

exception ErrorFromApplyingDefault of TcGlobals * DisplayEnv * Tast.Typar * TType * exn * range
exception ErrorFromAddingTypeEquation of TcGlobals * DisplayEnv * TType * TType * exn * range
exception ErrorsFromAddingSubsumptionConstraint of TcGlobals * DisplayEnv * TType * TType * exn * ContextInfo * range
exception ErrorFromAddingConstraint of DisplayEnv * exn * range
exception PossibleOverload of DisplayEnv * string * exn * range
exception UnresolvedOverloading of DisplayEnv * exn list * string * range
exception UnresolvedConversionOperator of DisplayEnv * TType * TType * range
exception ConstraintSolverTupleDiffLengths of displayEnv: DisplayEnv * TType list * TType list * range * range
Copy link
Contributor

@cartermp cartermp Apr 23, 2019

Choose a reason for hiding this comment

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

Nit: We don't normally have these sorts of whitespace additions. @dsyme what are your thoughts here?

Copy link
Contributor

@dsyme dsyme Apr 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I noticed that but let it slide, it's ok, it was inconsistent in the signature file in any case.

Copy link
Contributor Author

@smoothdeveloper smoothdeveloper Apr 24, 2019

Choose a reason for hiding this comment

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

Do you want me to clear the spacing in #6596, the overload related exceptions are changing.

Copy link
Contributor Author

@smoothdeveloper smoothdeveloper Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Could we clarify if, for example, such alignement is proscribed/discouraged/recommended/prescribed for specific or any situations in the guidelines:

https://github.com/Microsoft/visualfsharp/blob/master/CONTRIBUTING.md#coding-guidelines

Do the maintainers want to set a bar somewhere else than "code should have reasonable format, good effort on readability, and fit in the context"?

How does that get addressed in the guidelines & our CI/review workflow in the future?

I agree that code alignment makes editing slightly more involved in few cases, this is the main drawback I see, but the readability pros generally outweights it (subjective, for me).

I also agree the maintainers have the command on that when it comes to review/integrate contributions.

To look further around us, have you noticed jetbrains has been adding support for advanced settings for a bit: https://blog.jetbrains.com/dotnet/2017/11/28/aligning-code-columns-resharper-rider/ ?

It indicates that the demand for such features exist.

NB: This is not a rant 😄, let me know how to fix the code in the other PR.

Copy link
Contributor

@cartermp cartermp Apr 24, 2019

Choose a reason for hiding this comment

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

I personally am not a fan of column alignment (and an F#-specific variant, arrow aligment in match expressions, is not recommended as per the official style guide)

I don't think we have a hard and fast rule, but uniformity is what's preferred here above all else. That's why lots of whitespace normalization PRs were recently brought in.

Copy link
Contributor

@dsyme dsyme Apr 24, 2019

Choose a reason for hiding this comment

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

Yes we've been removing column alignment. There are some places we still use it though, e.g. tast.fs ValFlags, where it is extremely useful

Mostly we've been removing it to add space for doc comments, e.g.

let foo1 xxx yyy zzz = align
let foo2 x   y   z = align

becomes

/// comment
let foo1 xxx yyy zzz = align

/// comment
let foo2 x y z = align

Not much point in keeping the alignment when comments are added

exception ConstraintSolverInfiniteTypes of displayEnv: DisplayEnv * contextInfo: ContextInfo * TType * TType * range * range
exception ConstraintSolverTypesNotInEqualityRelation of displayEnv: DisplayEnv * TType * TType * range * range * ContextInfo
exception ConstraintSolverTypesNotInSubsumptionRelation of displayEnv: DisplayEnv * TType * TType * range * range
exception ConstraintSolverMissingConstraint of displayEnv: DisplayEnv * Tast.Typar * Tast.TyparConstraint * range * range
exception ConstraintSolverError of string * range * range
exception ConstraintSolverRelatedInformation of string option * range * exn

exception ErrorFromApplyingDefault of tcGlobals: TcGlobals * displayEnv: DisplayEnv * Tast.Typar * TType * exn * range
exception ErrorFromAddingTypeEquation of tcGlobals: TcGlobals * displayEnv: DisplayEnv * TType * TType * exn * range
exception ErrorsFromAddingSubsumptionConstraint of tcGlobals: TcGlobals * displayEnv: DisplayEnv * TType * TType * exn * ContextInfo * range
exception ErrorFromAddingConstraint of displayEnv: DisplayEnv * exn * range
exception PossibleOverload of displayEnv: DisplayEnv * string * exn * range
exception UnresolvedOverloading of displayEnv: DisplayEnv * exn list * string * range
exception UnresolvedConversionOperator of displayEnv: DisplayEnv * TType * TType * range

let GetPossibleOverloads amap m denv (calledMethGroup: (CalledMeth<_> * exn) list) =
calledMethGroup |> List.map (fun (cmeth, e) -> PossibleOverload(denv, NicePrint.stringOfMethInfo amap m denv cmeth.Method, e, m))
Expand Down Expand Up @@ -385,7 +385,7 @@ let ShowAccessDomain ad =
//-------------------------------------------------------------------------
// Solve

exception NonRigidTypar of DisplayEnv * string option * range * TType * TType * range
exception NonRigidTypar of displayEnv: DisplayEnv * string option * range * TType * TType * range
exception LocallyAbortOperationThatFailsToResolveOverload
exception LocallyAbortOperationThatLosesAbbrevs
let localAbortD = ErrorD LocallyAbortOperationThatLosesAbbrevs
Expand Down Expand Up @@ -724,7 +724,7 @@ let rec SolveTyparEqualsType (csenv: ConstraintSolverEnv) ndeep m2 (trace: Optio
// The types may still be equivalent due to abbreviations, which we are trying not to eliminate
if typeEquiv csenv.g ty1 ty then () else
// The famous 'occursCheck' check to catch "infinite types" like 'a = list<'a> - see also https://github.com/Microsoft/visualfsharp/issues/1170
if occursCheck csenv.g r ty then return! ErrorD (ConstraintSolverInfiniteTypes(csenv.eContextInfo, csenv.DisplayEnv, ty1, ty, m, m2)) else
if occursCheck csenv.g r ty then return! ErrorD (ConstraintSolverInfiniteTypes(csenv.DisplayEnv, csenv.eContextInfo, ty1, ty, m, m2)) else
// Note: warn _and_ continue!
do! CheckWarnIfRigid csenv ty1 r ty
// Record the solution before we solve the constraints, since
Expand Down
27 changes: 14 additions & 13 deletions src/fsharp/ConstraintSolver.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,21 +76,22 @@ type ContextInfo =
/// The type equation comes from a sequence expression.
| SequenceExpression of TType

exception ConstraintSolverTupleDiffLengths of DisplayEnv * TType list * TType list * range * range
exception ConstraintSolverInfiniteTypes of ContextInfo * DisplayEnv * TType * TType * range * range
exception ConstraintSolverTypesNotInEqualityRelation of DisplayEnv * TType * TType * range * range * ContextInfo
exception ConstraintSolverTypesNotInSubsumptionRelation of DisplayEnv * TType * TType * range * range
exception ConstraintSolverMissingConstraint of DisplayEnv * Typar * TyparConstraint * range * range
exception ConstraintSolverTupleDiffLengths of displayEnv: DisplayEnv * TType list * TType list * range * range
exception ConstraintSolverInfiniteTypes of displayEnv: DisplayEnv * contextInfo: ContextInfo * TType * TType * range * range
exception ConstraintSolverTypesNotInEqualityRelation of displayEnv: DisplayEnv * TType * TType * range * range * ContextInfo
exception ConstraintSolverTypesNotInSubsumptionRelation of displayEnv: DisplayEnv * TType * TType * range * range
exception ConstraintSolverMissingConstraint of displayEnv: DisplayEnv * Typar * TyparConstraint * range * range
exception ConstraintSolverError of string * range * range
exception ConstraintSolverRelatedInformation of string option * range * exn
exception ErrorFromApplyingDefault of TcGlobals * DisplayEnv * Typar * TType * exn * range
exception ErrorFromAddingTypeEquation of TcGlobals * DisplayEnv * TType * TType * exn * range
exception ErrorsFromAddingSubsumptionConstraint of TcGlobals * DisplayEnv * TType * TType * exn * ContextInfo * range
exception ErrorFromAddingConstraint of DisplayEnv * exn * range
exception UnresolvedConversionOperator of DisplayEnv * TType * TType * range
exception PossibleOverload of DisplayEnv * string * exn * range
exception UnresolvedOverloading of DisplayEnv * exn list * string * range
exception NonRigidTypar of DisplayEnv * string option * range * TType * TType * range

exception ErrorFromApplyingDefault of tcGlobals: TcGlobals * displayEnv: DisplayEnv * Typar * TType * exn * range
exception ErrorFromAddingTypeEquation of tcGlobals: TcGlobals * displayEnv: DisplayEnv * TType * TType * exn * range
exception ErrorsFromAddingSubsumptionConstraint of tcGlobals: TcGlobals * displayEnv: DisplayEnv * TType * TType * exn * ContextInfo * range
exception ErrorFromAddingConstraint of displayEnv: DisplayEnv * exn * range
exception UnresolvedConversionOperator of displayEnv: DisplayEnv * TType * TType * range
exception PossibleOverload of displayEnv: DisplayEnv * string * exn * range
exception UnresolvedOverloading of displayEnv: DisplayEnv * exn list * string * range
exception NonRigidTypar of displayEnv: DisplayEnv * string option * range * TType * TType * range

/// A function that denotes captured tcVal, Used in constraint solver and elsewhere to get appropriate expressions for a ValRef.
type TcValF = (ValRef -> ValUseFlag -> TType list -> range -> Expr * TType)
Expand Down