-
Notifications
You must be signed in to change notification settings - Fork 20
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 F# compiler directives like #nowarn to span less than an entire file. #278
Comments
Is this approved? I think it's important not to disable warnings unintentionally. Maybe something like:
or just:
|
@gusty, I agree, I really miss this feature in everyday development. Something like |
Is there a corresponding C# feature? (sorry - on mobile, haven't checked...) If so we should maintain alignment |
@dsyme, I believe we are quite behind with alignment when it comes down to compiler directives and similar statements. We support only a small amount of them. Think of In C# they don't have |
Having to disable many potential warnings in order to address a single one is an issue and AFAIK there is no workaround it. I'm currently being impacted by this issue in a real world project. |
C# does this well with #pragma warning. I'd vote to steal their approach,
as it seems like less friction when using both in solutions:
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives/preprocessor-pragma-warning
I like that you can disable multiple warnings, and reenable selectively if
needed.
…On Fri, May 4, 2018, 8:13 AM Gustavo Leon ***@***.***> wrote:
Having to disable many potential warnings in order to address a single one
is an issue and AFAIK there is no workaround it.
I'm currently being impacted by this issue in a real world project.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#278 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFCUmouiHRzyNEvuCu2tD_emP8Bealxuks5tvHACgaJpZM4KbqYL>
.
|
@ReedCopsey, I agree, with the caveat that we already have the While we're at it, I'd like to suggest alignment with |
@abelbraaksma, I am a little concerned about the scope of Explicit |
@dsyme, is this ready for a "needs rfc" status? |
@kkm000 Yes - For this sort of thing we're pretty much always happy to follow the C# approach to the same problem, as long as it is fully implemented and tested. |
To sum up the C# approach: it is not stack-based like MS C++
My take on this (I might try to put up an RFC, but just want to get a preliminary agreement on the approach):
It is not exactly clear where the let quite_a_long_function () =
. . .
let inner_function a b c =
let tuple = (let x = (a +
#nowarn "123"
(this_produces_a_warning b) ) *
#nowarn restore "123"
c in x*x, "foo")
Some tuple
. . . or a less contrived case that does happen in real life #nowarn "25" // Because we never call it with a None.
let f a (Some b) =
#nowarn restore "25"
(* 20 lines of implementation, where the incomplete match warning is beneficial. *) Here, the scope of It would be nice to have such a fine degree of control. Unfortunately, I'm not familiar with the parser internal handling of expressions, so I am just purely fantasizing; it might be such a complex thing to implement than its expense/benefit ratio would be questionable. Can anyone familiar with the compiler (@dsyme is perhaps the ultimate authority) please give a ballpark estimate of the complexity involved in handling the above examples? |
@kkm000 Great analysis thanks.
I think that depends entirely on your level of motivation and your ability to grok/change/test the somewhat messy code that processes these things for scripts, compiler service and regular processing. Having |
It would be nice if these pragmas could follow normal scoping rules. |
I'd agree, but I think scoping is done way later in the process (not sure, just assuming), and pre-processing directives are typically processed as early as possible. In fact, there are currently severe problems with some preprocessing directives when you accidentally use them scoped (that is, indented). I reported them somewhere, but it has low priority on the long list of todos, I think.
@kkm000, yes, it would. But that very point should actually remove your concerns, as the current syntax applies to a whole file, and one alternative approach ( The difference being that @dsyme, @kkm000, I think I saw a PR somewhere (and maybe an RFC?) that introduces new syntax, can that be linked here? |
Related request on |
Any updates? |
This is approved in principle so I guess someone have to draft an RFC. |
To clarify, what exactly is approved? Just adding |
I hope the latter. I think it’s time to go over the directives in general and align where possible or sensible. |
I self-assigned this, I hope to get to writing an RFC soon(ish), as this keeps popping up. |
One option to consider for the closing/restoring directive for Just as the A Scoped disabling with
|
@brianrourkeboll I agree to the general approach you lay out, as that ties in with the general approach that C# has taken, see #278 (comment). And when used without arguments, I'd have to check how they deal with commandline arguments in the C# case though. I'd assume these are not touched and that the scoped, file-based ones only apply to that particular file, where the ones on the commandline apply to the whole project. |
This was proposed in 2016, it would be huge if we could get this. Is there a way we can pay for a bounty on things like this because I'm tired of having warnings that I have to see every day because a different area of the file might need it. I know the devil is in the details and it's probably not as easy as it looks, and I'm not trying to judge you for not getting around to it, I just wish there were a way to help even though I lack the availability to contribute. |
@voronoipotato actually - there is a way to do just that :) |
I will have a look at the feasibility, since with https://github.com/fsharp/fslang-design/blob/main/RFCs/FS-1060-nullable-reference-types.md in sight the desire to have this will increase, especially for gradual introduction of this feature into big projects. But it still is a feature which needs an RFC, documentation updates and an announcement with motivating examples, so any kind of help (even non-programmatical), is welcomed. |
The place to add this, considering only the override _.DiagnosticSink(diagnostic: PhasedDiagnostic, severity) =
if severity = FSharpDiagnosticSeverity.Error then
realErrorPresent <- true
diagnosticsLogger.DiagnosticSink(diagnostic, severity)
else
let report =
let warningNum = diagnostic.Number
match diagnostic.Range with
| Some m ->
scopedPragmas
|> List.exists (fun pragma ->
let (ScopedPragma.WarningOff(pragmaRange, warningNumFromPragma)) = pragma
warningNum = warningNumFromPragma
&& (not checkFile || m.FileIndex = pragmaRange.FileIndex)
&& posGeq m.Start pragmaRange.Start)
|> not
| None -> true As you can see, the existing code already checks for range of the "opening" If someone wants to introduce this feature, I can help/advice/pair on the implementation and testing. |
I think any support would be great, and then if someone can get full alignment that would also be nice but being able to nowarn a few lines is most critical for my workplace.
|
If nobody else has volunteered, I would want to tackle this end-to-end. |
@Martin521 I've got a lot on my plate so unless I can bargain for some research time at work for this I can't meaningfully take it on. If you have a tip jar though for your efforts I'd be happy to pitch in. |
@Martin521 I wonder if it will be easier to just post here, because I know Tomáš will be off next week, and might not have time before then, however I or someone else can answer questions here. Or if you'd like to, you can ping me, and we can chat. |
I do actually agree with this one, it should definitely first align with our own compiler flags, and aligning with C# is orthogonal. |
I think that Consider a scenario like this: #nowarn "25" // Incomplete pattern matches.
let [x] = List.singleton 1
#warnon Is So I prefer my original suggestion, i.e., that #nowarn "25" // Incomplete pattern matches.
let [x] = List.singleton 1
#warnon "25" // Incomplete pattern matches. |
I would personally expect this to not work (i.e. expect #warnon to be explicit and require a number/set of numbers). Update: Yep, as you wrote above:
|
Yes, that was also what I thought.
This is a hobby project for me, so it may take some time. Here are my questions:
No need to answer all of the questions, I can find out the technical stuff. Advice on the location for the tests and regarding expression scope would be good. |
Currently, it doesn't seem like it (nowarn disabled warning above it):
I think we should, yes.
Yeah, I couldn't either, at least not dedicated category.
Probably only one for now - since now
I would ping @auduchinok here, he knows more than I do here.
Probably we shouldn't as part of this PR, since people could've used them as some sort of markers around the code and it might be a breaking change for them.
|
Funny enough, we hit wanting for this yesterday. Our use case was we have a set of errors, one of which has been obsoleted. type MyErrors
| CoolError1 of CoolError1
| CoolError2 of CoolError2
| [<Obsolete>] OldError1 of OldError1 And it would be great to do only small sections that we know we want to ignore rather than the whole file like: let handler e =
match e with
| CoolError1 ce -> ...
| CoolError2 ce -> ...
#nowarn 44
| OldError ce -> ...
#warnon 44
Yeah depending on where the |
@vzarytovskii it's breaking in that behavior is changing, but it is not breaking in that it would interrupt anyone's development process. They would just see a few warnings. If they had the impression that it was working the way we would like (as I suspect most do), they would see warnings that they want to see, and haven't been. The current user experience of "nowarn anywhere in the file causes all warnings to be hidden" is surprising enough that I doubt people who put it in the middle of a file intended to hide warnings above it. In my opinion we should just fix it. |
It is breaking in a way that something which was producing no errors in IDE before, might start producing them (given that majority turn warnings as errors). |
Yes, a new case for
|
The population of people who put nowarn in the middle of a file and had it marked warning as error, and were trying to nowarn the entire file in my intuition is smaller than the population of people who put nowarn in the middle of the file, marked warning as error, and were trying to nowarn a portion of the file. The current way it works is very surprising and I have been burned by it more than once. I understand that it's "technically" still breaking, but my point is that the existing functionality is effectively "not working" in terms of what many people will expect. |
There is now a RFC proposal for this feature |
Submitted by Robert Nielsen on 6/22/2014 12:00:00 AM
7 votes on UserVoice prior to migration
Idea posted here:
http://visualstudio.uservoice.com/forums/121579-visual-studio/suggestions/3338653-allow-f-compiler-directives-like-nowarn-to-span
A possibly related issue / suggestion is listed here:
/archive/suggestion-5676502-disable-compiler-warnings-per-line
Original UserVoice Submission
Archived Uservoice Comments
The text was updated successfully, but these errors were encountered: