Fix issue 18361: [ddoc] redirect automatically highlighted text in re…

[quickfur]

…placeable macros.

Basically, this is to give the user the ability to opt out of automatic
keyword highlighting in doc text, in the cases where a proliferation of
_s would be needed to suppress this behaviour.

The default behaviour is not changed; the default ddoc theme will still
redirect the new macros to the old DDOC_PSYMBOL, DDOC_KEYWORD,
DDOC_PARAM.

[dlang-bot]

Thanks for your pull request, @quickfur!

Bugzilla references

Auto-close	Bugzilla	Description
✓	18361	Ddoc: support ability to opt out of automatic keyword highlighting in text

[jmdavis]

Honestly, I think that we'd be better off just killing the auto-highlighting entirely. It may have sounded like a good idea, but all in all, it's caused us nothing but grief. And if we leave it as opt-out, then it's still going to be causing us grief, much as I agree that being able to opt-out is an improvement over the status quo.

[wilzbach]

Could come with a short changelog explaining how to opt-out.

[wilzbach]

I agree that being able to opt-out is an improvement over the status quo.

I think we should merge this directly and put the discussion of whether or not to turn the trigger by default on a separate PR.

@quickfur it would need a simple PR to https://dlang.org/spec/ddoc.html#macros before it can be merged. Also I would add a few lines to changelog to show users how they can opt-out. The single line bug fixes get easily lost in the changelog.

Honestly, I think that we'd be better off just killing the auto-highlighting entirely. It may have sounded like a good idea, but all in all, it's caused us nothing but grief.

Agreed (I also vote for killing it with 🔥), but that would require approval from @WalterBright and/or @andralex.

[adamdruppe]

the thing is you can opt out right now by defining DDOC_PSYMBOL=$0

The tricky thing with migrating to sanity is removing the excess _ from existing code. Good news is that by "tricky" I mean "trivial": just make the compiler tell you when it detects those and remove them all. Then we can kill it. And it isn't strictly wrong to leave them there for now but we'd want to remove them before long.

But regardless anything that gets us closer to killing this once and for all is good.

[quickfur]

@jmdavis I agree, but this way is less controversial and at least gives us the worst-case scenario where we can opt out even if everything else gets vetoed.

@wilzbach OK, I'll add a dlang.org PR. Yeah, I'd also vote for killing this with fire... or, like the beginning of the movie Outbreak, send an airplane over to drop a "package"... :-D

@adamdruppe The problem with nulling out DDOC_PSYMBOL is that it is legitimately used inside code blocks to highlight function parameters. And also, auto-highlighted text includes module symbols and keywords like false, and those also have legitimate highlighting inside code blocks. So to kill off the _ contagion without collateral damage, this PR's approach is a necessary first step.

But anyway, killing it off is not hard at all; you don't even need any special addition to the compiler. Just redefine DDOC_AUTO_PSYMBOL, et al, to something like BIGFATREDWARNINGFIXMEPLZZ $0 and generate your ddocs once, then look over the output for all occurrences of BIGFATREDWARNINGFIXMEPLZZ which will be highly visible, and fix all the corresponding places in the code. :-P

[adamdruppe]

But anyway, killing it off is not hard at all; you don't even need any special addition to the compiler. Just redefine DDOC_AUTO_PSYMBOL, et al, to something like BIGFATREDWARNINGFIXMEPLZZ $0 and generate your ddocs once, then look over the output for all occurrences of BIGFATREDWARNINGFIXMEPLZZ which will be highly visible, and fix all the corresponding places in the code. :-P

That's the opposite of what to fix. To fix the source code, you aren't concerned about the places where DDOC_AUTO_PSYMBOL is triggered but rather the places where it is suppressed by the leading underscore.

The compiler knows these places though, and it could also be defined to a macro or whatever. It is this line slightly higher than your diff:
https://github.com/quickfur/dmd/blob/9b439afcb10fa81c76cc1a544133f387079fe289/src/dmd/doc.d#L2471

that indicates source code to change.

So once you defined DDOC_AUTO_PSYMBOL=$0, then you will want to search your code and remove the leading _, triggered by the code in the above link. That will clean up the source from the misfeature and ultimately provide a path to remove the whole highlight thing entirely. (if it is removed entirely, those leading _ will be rendered in the final output if not cleaned up)

[andralex]

We should define the macro DDOC_AUTO_PSYMBOL_SUPPRESS, inserted whenever the underscore is used to suppress.

[quickfur]

dlang.org PR: dlang/dlang.org#2168

[adamdruppe]

@quickfur i think you have to i-- on the new set of the diff before you bracket to fix the autotester. (I used to know this code quite well back when I was enthusiastic about fixing ddoc but that was years ago)

[quickfur]

Hmm. Actually, with this PR, even if you redefine DDOC_AUTO_PSYMBOL to pass its argument through unhighlighted, none of the underscore prefixes will show up, because they are still being processed by the suppression code. So old occurrences of suppressed words will still show up correctly, just that you no longer need to prefix words with _ going forward.

However, I still added DDOC_AUTO_PSYMBOL_SUPPRESS to aid users who would like to eliminate those old redundant _s from their code anyway, even if they don't actually lead to rendering artifacts.

[quickfur]

@adamdruppe Hmm, lemme run some tests to see what the code does. I just copied-n-pasted the bracketing code from below, but you're right, it may not quite work correctly because of the buf.remove(i, 1).

P.S. Well, I really should be adding test cases to ensure nothing breaks in the future. But I'll have to work on that tomorrow or Monday... going to be busy now.

[quickfur]

These overrides are to reduce the scope of the test so that the output HTML does not depend on the styling of the outer elements, that are unrelated to this PR. So that later ddoc PRs won't necessitate updating the test output file unnecessarily.

[quickfur]

OK I lied, managed to squeeze in the time to add a test case. Destroy!

[WalterBright]

It's ok with me if the _ highlighting is removed. All I can say is it seemed like a good idea at the time.

[jmdavis]

It's ok with me if the _ highlighting is removed. All I can say is it seemed like a good idea at the time.

I can totally understand it seeming like a good idea at the time, but I do think that experience has shown that it ultimately wasn't a good idea. We've all made mistakes like that though.

[wilzbach]

help?

[ibuclaw]

repressed?

[quickfur]

Sorry, it was intended as a joke. The misspelling is intentional. It's basically supposed to be any string that's hard to accidentally reproduce or coincide with anything else, that stands out in the output so that we know for certain that the code is working as expected.

[adamdruppe]

I think the big mistake with the original thing was that it was opt in instead of opt out. So it would hit a LOT of false positives as variable names and English text coincided.

But if it was opt in, it could have been the basis of a nice cross reference thing like mine http://dpldocs.info/experimental-docs/adrdox.syntax.html#cross-referencing

Perhaps once we remove the automatic thing, ddoc can use the symbol lookup code later for a reference thing.

[quickfur]

@WalterBright It's no one's fault; hindsight is always 20/20. But at the time, it was probably not so easy to anticipate the problems that would arise later.

[quickfur]

Fixed bug in test case that's causing autotester failure.

[wilzbach]

@quickfur it's still segfaulting:

/home/braddr/sandbox/at-client/pull-3020604-Linux_64_64/dmd/generated/linux/release/64/dmd -conf= -m64 -Icompilable -D -Ddgenerated/compilable -o-  -fPIC  -odgenerated/compilable -ofgenerated/compilable/ddoc11823_0.o  -c compilable/ddoc11823.d 
Segmentation fault (core dumped)

[quickfur]

Ahh, it's a different test that's segfaulting. I was wondering why the autotester segfaults when running my test locally doesn't. I'll look into it.

[quickfur]

@adamdruppe was right: there's an off-by-one error that's causing the segfault. Should be fixed now.

[wilzbach]

object.Exception@../tools/changed.d(210): Changelog entries should consist of one title line, a blank separator line, and a description.
----------------

[wilzbach]

(fixed it myself)

[wilzbach]

So DAutoTest complains correctly that there are two undefined macros now:

/dev/shm/dtest/work/repo/dlang.org/web/phobos-prerelease/std_net_curl.html:818:<div class="keyval SeeAlso"><span class="key keySeeAlso">See Also:</span> <div class="val valSeeAlso">$(HTTP www.ietf.org/rfc/rfc2616.txt, RFC2616)</div></div>
/dev/shm/dtest/work/repo/dlang.org/web/phobos-prerelease/std_math.html:264:<dd><div class="summary">$(PI) = 3.141592...</div>

-> we need to fix Phobos first: dlang/phobos#6149