Reliable getTypeLink. #380
Conversation
For let getTypeLink (ctx:ReadingContext) undefinedLink Before the change, when the link is undefined, the FSharp.Formatting will crash. The change will allow an undefinedLink to return None, and let the rest of the parsing completes.
|
Thanks a lot for this! Is the call throwing any more specific exception, or do we need to catch all possible exceptions? The change looks good to me & I'll merge it, but if it throws something more specific, it would be good to handle that more precisely... |
|
It is throw by: resolveCref invalidArg "cref" (sprintf "the given cref: '%s' is invalid!" cref) which crashes the program. The particular instance show the exception as: the given cref: 'T:' is invalid!" so cref is “T:”. Generally, it will be much more helpful if FSharp.Formatting can print the line # and docs where the above issue occurs. The above exception when thrown doesn’t give any context on where the issue is located. It made me scratch my head and can’t figure out which documentation formatting is done right. Thanks, From: Tomas Petricek [mailto:[email protected]] Thanks a lot for this! Is the call throwing any more specific exception, or do we need to catch all possible exceptions? The change looks good to me & I'll merge it, but if it throws something more specific, it would be good to handle that more precisely... — |
|
So this happens when I don't feel like this is the correct fix, we should either change this condition to |
|
At the build when this triggers, it doesn’t print out the comment text (or the surrounding text in XML) that produce the issue. I have yet to figure out what comment triggers this. Best, From: Matthias Dittrich [mailto:[email protected]] So this happens when undefinedLink is empty. I would really like to know in which situations that happens. @jinlmsft https://github.com/jinlmsft can you provide a unit test for this (or a example where this happens)? I don't feel like this is the correct fix, we should either change this https://github.com/jinlmsft/FSharp.Formatting/blob/968ebac773f34ad7dee2284913bfed39d4c6d045/src/FSharp.MetadataFormat/Main.fs#L893 condition to < instead of <=. But at the very least we should log the exception instead of just hiding it. — |
|
Ok I think I figured it out: The problem is markup like because the whitespace will be trimmed and then the "code" will be an empty string. |
|
Can you clarify if 1) the comment need to be written in a corrected way, or 2) the program need to tolerate an empty string? If the case is 1), it will be helpful to print out the XML line number that the problem occurs. As a developer, I can then figure out what inline comment needs to be properly formatted. If the case is 2), then it will be great to catch the exception, and not expose it to the developer. Thanks, From: Matthias Dittrich [mailto:[email protected]] Ok I think I figured it out: The problem is markup like Some empty because the whitespace will be trimmed and then the "code" will be an empty string. — |
|
The behavior repo again: You may clone: https://github.com/jinlmsft/Prajna/ and switch to branch FailedCommentXML. Please use: .\build.cmd Release To build. During document generation, the exception will fire: System.ArgumentException: the given cref: 'T:' is invalid! Parameter name: cref at FSharp.MetadataFormat.Reader.resolveCref@887(Dictionary y`2 niceNameEntityLookup, String cref) in c:\Tomas\Public\tpetricek\FSharp.Formatting\src\FSharp.MetadataFormat\Main.fs: line 888 Thanks, From: Jin MSFT [mailto:[email protected]] Can you clarify if 1) the comment need to be written in a corrected way, or 2) the program need to tolerate an empty string? If the case is 1), it will be helpful to print out the XML line number that the problem occurs. As a developer, I can then figure out what inline comment needs to be properly formatted. If the case is 2), then it will be great to catch the exception, and not expose it to the developer. Thanks, From: Matthias Dittrich [mailto:[email protected]] Ok I think I figured it out: The problem is markup like Some empty because the whitespace will be trimmed and then the "code" will be an empty string. — |
|
To clarify: 2) The tool needs to properly handle this case. FYI: The problem there is that we need to do some major code changes (and probably a lot of breaking changes) to add line/column information to the parsed data structures. While we can probably improve the situation for the standalone markup parser (with some additional arguments) we cannot simply do the same for the Literate and the Metadata libraries. |
|
It you can print the surrounding text in XML that cause the exception, that may be very helpful as well. I can then do a search in the XML file to find what causes the problem. Jin From: Matthias Dittrich [mailto:[email protected]] To clarify: 2) The tool needs to properly handle this case. FYI: The problem there is that we need to do some major code changes (and probably a lot of breaking changes) to add line/column information to the parsed data structures. While we can probably improve the situation for the standalone markup parser (with some additional arguments) we cannot simply do the same for the Literate and the Metadata libraries. — |
add test for #380 and change the fix.
|
@jinlmsft The exception should no longer happen in the current (and future) versions :) |
For
let getTypeLink (ctx:ReadingContext) undefinedLink
Before the change, when the link is undefined, the FSharp.Formatting will crash. The change will allow an undefinedLink to return None, and let the rest of the parsing completes.